Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n
@ 2018-01-15 17:30 Max Kellermann
  2018-01-15 17:30 ` [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com> Max Kellermann
  2018-01-17 16:37 ` [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n J. Bruce Fields
  0 siblings, 2 replies; 6+ messages in thread
From: Max Kellermann @ 2018-01-15 17:30 UTC (permalink / raw)
  To: linux-fsdevel, hch, linux-nfs, trond.myklebust, gregkh
  Cc: max.kellermann, linux-kernel

Make IS_POSIXACL() return false if POSIX ACL support is disabled and
ignore SB_POSIXACL/MS_POSIXACL.

Never skip applying the umask in namei.c and never bother to do any
ACL specific checks if the filesystem falsely indicates it has ACLs
enabled when the feature is completely disabled in the kernel.

This fixes a problem where the umask is always ignored in the NFS
client when compiled without CONFIG_FS_POSIX_ACL.  This is a 4 year
old regression caused by commit 013cdf1088d723 which itself was not
completely wrong, but failed to consider all the side effects by
misdesigned VFS code.

Prior to that commit, there were two places where the umask could be
applied, for example when creating a directory:

 1. in the VFS layer in SYSCALL_DEFINE3(mkdirat), but only if
    !IS_POSIXACL()

 2. again (unconditionally) in nfs3_proc_mkdir()

The first one does not apply, because even without
CONFIG_FS_POSIX_ACL, the NFS client sets MS_POSIXACL in
nfs_fill_super().

After that commit, (2.) was replaced by:

 2b. in posix_acl_create(), called by nfs3_proc_mkdir()

There's one branch in posix_acl_create() which applies the umask;
however, without CONFIG_FS_POSIX_ACL, posix_acl_create() is an empty
dummy function which does not apply the umask.

The approach chosen by this patch is to make IS_POSIXACL() always
return false when POSIX ACL support is disabled, so the umask always
gets applied by the VFS layer.  This is consistent with the (regular)
behavior of posix_acl_create(): that function returns early if
IS_POSIXACL() is false, before applying the umask.

Therefore, posix_acl_create() is responsible for applying the umask if
there is ACL support enabled in the file system (SB_POSIXACL), and the
VFS layer is responsible for all other cases (no SB_POSIXACL or no
CONFIG_FS_POSIX_ACL).

Signed-off-by: Max Kellermann <mk@cm4all.com>
---
 include/linux/fs.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 440281f8564d..c3240c28e61b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1883,7 +1883,12 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
 #define IS_NOQUOTA(inode)	((inode)->i_flags & S_NOQUOTA)
 #define IS_APPEND(inode)	((inode)->i_flags & S_APPEND)
 #define IS_IMMUTABLE(inode)	((inode)->i_flags & S_IMMUTABLE)
+
+#ifdef CONFIG_FS_POSIX_ACL
 #define IS_POSIXACL(inode)	__IS_FLG(inode, SB_POSIXACL)
+#else
+#define IS_POSIXACL(inode)	0
+#endif
 
 #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
 #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com>
  2018-01-15 17:30 [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n Max Kellermann
@ 2018-01-15 17:30 ` Max Kellermann
  2018-01-15 17:41   ` Greg KH
  2018-01-16 15:06   ` Trond Myklebust
  2018-01-17 16:37 ` [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n J. Bruce Fields
  1 sibling, 2 replies; 6+ messages in thread
From: Max Kellermann @ 2018-01-15 17:30 UTC (permalink / raw)
  To: linux-fsdevel, hch, linux-nfs, trond.myklebust, gregkh
  Cc: max.kellermann, linux-kernel

nfs/super: set MS_POSIXACL only if ACL support is enabled

The code comment says "We will [apply the umask] ourselves", but that
happens in posix_acl_create() only if the kernel has POSIX ACL
support.  Without it, posix_acl_create() is a is an empty dummy
function.

So let's not pretend we will apply the umask if we can already know
that we will never.

This fixes a problem where the umask is always ignored in the NFS
client when compiled without CONFIG_FS_POSIX_ACL.  This is a 4 year
old regression caused by commit 013cdf1088d723 which itself was not
completely wrong, but failed to consider all the side effects by
misdesigned VFS code.

There are two compile-time checks and one runtime check:

- If CONFIG_FS_POSIX_ACL=n, then MS_POSIXACL is never set.

- If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=n, then only NFSv4
  has ACL support (and cannot be disabled), and we need to check for
  "version==4".

- If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=y, MS_POSIXACL is
  always set, as before.

Signed-off-by: Max Kellermann <mk@cm4all.com>
---
 fs/nfs/super.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 216f67d628b3..ec4e1f2775e0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2338,10 +2338,17 @@ void nfs_fill_super(struct super_block *sb, struct nfs_mount_info *mount_info)
 		sb->s_blocksize = nfs_block_size(data->bsize, &sb->s_blocksize_bits);
 
 	if (server->nfs_client->rpc_ops->version != 2) {
-		/* The VFS shouldn't apply the umask to mode bits. We will do
-		 * so ourselves when necessary.
-		 */
-		sb->s_flags |= MS_POSIXACL;
+#ifdef CONFIG_FS_POSIX_ACL
+#ifndef CONFIG_NFS_V3_ACL
+		if (nfss->nfs_client->rpc_ops->version == 4)
+#endif
+			/* The VFS shouldn't apply the umask to mode
+			 * bits. We will do so ourselves when
+			 * necessary.
+			 */
+			sb->s_flags |= MS_POSIXACL;
+#endif
+
 		sb->s_time_gran = 1;
 		sb->s_export_op = &nfs_export_ops;
 	}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com>
  2018-01-15 17:30 ` [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com> Max Kellermann
@ 2018-01-15 17:41   ` Greg KH
  2018-01-15 17:43     ` Max Kellermann
  2018-01-16 15:06   ` Trond Myklebust
  1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2018-01-15 17:41 UTC (permalink / raw)
  To: Max Kellermann
  Cc: linux-fsdevel, hch, linux-nfs, trond.myklebust, max.kellermann,
	linux-kernel

On Mon, Jan 15, 2018 at 06:30:51PM +0100, Max Kellermann wrote:
> nfs/super: set MS_POSIXACL only if ACL support is enabled
> 
> The code comment says "We will [apply the umask] ourselves", but that
> happens in posix_acl_create() only if the kernel has POSIX ACL
> support.  Without it, posix_acl_create() is a is an empty dummy
> function.

<snip>

Your subject line is a bit odd :(

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com>
  2018-01-15 17:41   ` Greg KH
@ 2018-01-15 17:43     ` Max Kellermann
  0 siblings, 0 replies; 6+ messages in thread
From: Max Kellermann @ 2018-01-15 17:43 UTC (permalink / raw)
  To: Greg KH
  Cc: Max Kellermann, linux-fsdevel, hch, linux-nfs, trond.myklebust,
	max.kellermann, linux-kernel

On 2018/01/15 18:41, Greg KH <gregkh@linuxfoundation.org> wrote:
> Your subject line is a bit odd :(

True, I already repaired & resent it.  Sorry for the hiccup.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com>
  2018-01-15 17:30 ` [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com> Max Kellermann
  2018-01-15 17:41   ` Greg KH
@ 2018-01-16 15:06   ` Trond Myklebust
  1 sibling, 0 replies; 6+ messages in thread
From: Trond Myklebust @ 2018-01-16 15:06 UTC (permalink / raw)
  To: hch, mk, linux-nfs, gregkh, linux-fsdevel; +Cc: max.kellermann, linux-kernel

On Mon, 2018-01-15 at 18:30 +0100, Max Kellermann wrote:
> nfs/super: set MS_POSIXACL only if ACL support is enabled
> 
> The code comment says "We will [apply the umask] ourselves", but that
> happens in posix_acl_create() only if the kernel has POSIX ACL
> support.  Without it, posix_acl_create() is a is an empty dummy
> function.
> 
> So let's not pretend we will apply the umask if we can already know
> that we will never.
> 
> This fixes a problem where the umask is always ignored in the NFS
> client when compiled without CONFIG_FS_POSIX_ACL.  This is a 4 year
> old regression caused by commit 013cdf1088d723 which itself was not
> completely wrong, but failed to consider all the side effects by
> misdesigned VFS code.
> 
> There are two compile-time checks and one runtime check:
> 
> - If CONFIG_FS_POSIX_ACL=n, then MS_POSIXACL is never set.
> 
> - If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=n, then only NFSv4
>   has ACL support (and cannot be disabled), and we need to check for
>   "version==4".
> 
> - If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=y, MS_POSIXACL is
>   always set, as before.
> 
> Signed-off-by: Max Kellermann <mk@cm4all.com>
> ---
>  fs/nfs/super.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 216f67d628b3..ec4e1f2775e0 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -2338,10 +2338,17 @@ void nfs_fill_super(struct super_block *sb,
> struct nfs_mount_info *mount_info)
>  		sb->s_blocksize = nfs_block_size(data->bsize, &sb-
> >s_blocksize_bits);
>  
>  	if (server->nfs_client->rpc_ops->version != 2) {
> -		/* The VFS shouldn't apply the umask to mode bits.
> We will do
> -		 * so ourselves when necessary.
> -		 */
> -		sb->s_flags |= MS_POSIXACL;
> +#ifdef CONFIG_FS_POSIX_ACL
> +#ifndef CONFIG_NFS_V3_ACL
> +		if (nfss->nfs_client->rpc_ops->version == 4)
> +#endif
> +			/* The VFS shouldn't apply the umask to mode
> +			 * bits. We will do so ourselves when
> +			 * necessary.
> +			 */
> +			sb->s_flags |= MS_POSIXACL;
> +#endif
> +
>  		sb->s_time_gran = 1;
>  		sb->s_export_op = &nfs_export_ops;
>  	}

The above illustrates exactly why I've asked people _never_ to make
anything conditional on rpc_ops->version. Please use a NFS capability
(i.e. NFS_SB(sb)->caps) for this kind of thing. That expresses the
condition in terms of the functionality we want instead of a whimsical
protocol version number.

Thanks
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n
  2018-01-15 17:30 [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n Max Kellermann
  2018-01-15 17:30 ` [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com> Max Kellermann
@ 2018-01-17 16:37 ` J. Bruce Fields
  1 sibling, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2018-01-17 16:37 UTC (permalink / raw)
  To: Max Kellermann
  Cc: linux-fsdevel, hch, linux-nfs, trond.myklebust, gregkh,
	max.kellermann, linux-kernel, agreunba, Al Viro

Looks right to me.

Reviewed-by: J. Bruce Fields <bfields@redhat.com>

--b.

On Mon, Jan 15, 2018 at 06:30:46PM +0100, Max Kellermann wrote:
> Make IS_POSIXACL() return false if POSIX ACL support is disabled and
> ignore SB_POSIXACL/MS_POSIXACL.
> 
> Never skip applying the umask in namei.c and never bother to do any
> ACL specific checks if the filesystem falsely indicates it has ACLs
> enabled when the feature is completely disabled in the kernel.
> 
> This fixes a problem where the umask is always ignored in the NFS
> client when compiled without CONFIG_FS_POSIX_ACL.  This is a 4 year
> old regression caused by commit 013cdf1088d723 which itself was not
> completely wrong, but failed to consider all the side effects by
> misdesigned VFS code.
> 
> Prior to that commit, there were two places where the umask could be
> applied, for example when creating a directory:
> 
>  1. in the VFS layer in SYSCALL_DEFINE3(mkdirat), but only if
>     !IS_POSIXACL()
> 
>  2. again (unconditionally) in nfs3_proc_mkdir()
> 
> The first one does not apply, because even without
> CONFIG_FS_POSIX_ACL, the NFS client sets MS_POSIXACL in
> nfs_fill_super().
> 
> After that commit, (2.) was replaced by:
> 
>  2b. in posix_acl_create(), called by nfs3_proc_mkdir()
> 
> There's one branch in posix_acl_create() which applies the umask;
> however, without CONFIG_FS_POSIX_ACL, posix_acl_create() is an empty
> dummy function which does not apply the umask.
> 
> The approach chosen by this patch is to make IS_POSIXACL() always
> return false when POSIX ACL support is disabled, so the umask always
> gets applied by the VFS layer.  This is consistent with the (regular)
> behavior of posix_acl_create(): that function returns early if
> IS_POSIXACL() is false, before applying the umask.
> 
> Therefore, posix_acl_create() is responsible for applying the umask if
> there is ACL support enabled in the file system (SB_POSIXACL), and the
> VFS layer is responsible for all other cases (no SB_POSIXACL or no
> CONFIG_FS_POSIX_ACL).
> 
> Signed-off-by: Max Kellermann <mk@cm4all.com>
> ---
>  include/linux/fs.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 440281f8564d..c3240c28e61b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1883,7 +1883,12 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
>  #define IS_NOQUOTA(inode)	((inode)->i_flags & S_NOQUOTA)
>  #define IS_APPEND(inode)	((inode)->i_flags & S_APPEND)
>  #define IS_IMMUTABLE(inode)	((inode)->i_flags & S_IMMUTABLE)
> +
> +#ifdef CONFIG_FS_POSIX_ACL
>  #define IS_POSIXACL(inode)	__IS_FLG(inode, SB_POSIXACL)
> +#else
> +#define IS_POSIXACL(inode)	0
> +#endif
>  
>  #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
>  #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-01-17 16:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-15 17:30 [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n Max Kellermann
2018-01-15 17:30 ` [PATCH 2/2] dFrom: Max Kellermann <mk@cm4all.com> Max Kellermann
2018-01-15 17:41   ` Greg KH
2018-01-15 17:43     ` Max Kellermann
2018-01-16 15:06   ` Trond Myklebust
2018-01-17 16:37 ` [PATCH 1/2] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n J. Bruce Fields

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).