[PATCH v3 1/4] fs/posix_acl: apply umask if superblock disables ACL support

[PATCH v3 1/4] fs/posix_acl: apply umask if superblock disables ACL support

[Max Kellermann]

The function posix_acl_create() applies the umask only if the inode
has no ACL (= NULL) or if ACLs are not supported by the filesystem
driver (= -EOPNOTSUPP).

However, this happens only after after the IS_POSIXACL() check
succeeded.  If the superblock doesn't enable ACL support, umask will
never be applied.  A filesystem which has no ACL support will of
course not enable SB_POSIXACL, rendering the umask-applying code path
unreachable.

This fixes a bug which causes the umask to be ignored with O_TMPFILE
on tmpfs:

 https://github.com/MusicPlayerDaemon/MPD/issues/558
 https://bugs.gentoo.org/show_bug.cgi?id=686142#c3
 https://bugzilla.kernel.org/show_bug.cgi?id=203625

Signed-off-by: Max Kellermann <mk@cm4all.com>
Reviewed-by: J. Bruce Fields <bfields@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/posix_acl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 249672bf54fe..e5e7a2295b99 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -589,9 +589,14 @@ posix_acl_create(struct inode *dir, umode_t *mode,
 	*acl = NULL;
 	*default_acl = NULL;
 
-	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
+	if (S_ISLNK(*mode))
 		return 0;
 
+	if (!IS_POSIXACL(dir)) {
+		*mode &= ~current_umask();
+		return 0;
+	}
+
 	p = get_acl(dir, ACL_TYPE_DEFAULT);
 	if (!p || p == ERR_PTR(-EOPNOTSUPP)) {
 		*mode &= ~current_umask();
-- 
2.20.1

[PATCH v3 2/4] fs/ext4/acl: apply umask if ACL support is disabled

[Max Kellermann]

The function ext4_init_acl() calls posix_acl_create() which is
responsible for applying the umask.  But without
CONFIG_EXT4_FS_POSIX_ACL, ext4_init_acl() is an empty inline function,
and nobody applies the umask.

This fixes a bug which causes the umask to be ignored with O_TMPFILE
on ext4:

 https://github.com/MusicPlayerDaemon/MPD/issues/558
 https://bugs.gentoo.org/show_bug.cgi?id=686142#c3
 https://bugzilla.kernel.org/show_bug.cgi?id=203625

Signed-off-by: Max Kellermann <mk@cm4all.com>
Reviewed-by: J. Bruce Fields <bfields@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/ext4/acl.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
index 9b63f5416a2f..7f3b25b3fa6d 100644
--- a/fs/ext4/acl.h
+++ b/fs/ext4/acl.h
@@ -67,6 +67,11 @@ extern int ext4_init_acl(handle_t *, struct inode *, struct inode *);
 static inline int
 ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
 {
+	/* usually, the umask is applied by posix_acl_create(), but if
+	   ext4 ACL support is disabled at compile time, we need to do
+	   it here, because posix_acl_create() will never be called */
+	inode->i_mode &= ~current_umask();
+
 	return 0;
 }
 #endif  /* CONFIG_EXT4_FS_POSIX_ACL */
-- 
2.20.1

[PATCH v3 3/4] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n

[Max Kellermann]

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>
Reviewed-by: J. Bruce Fields <bfields@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
---
 include/linux/fs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index abedbffe2c9e..5721be1146b1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2027,7 +2027,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)
-- 
2.20.1

[PATCH v3 4/4] nfs/super: check NFS_CAP_ACLS instead of the NFS version

[Max Kellermann]

This sets SB_POSIXACL only if ACL support is really enabled, instead
of always setting SB_POSIXACL if the NFS protocol version
theoretically supports ACL.

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 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.

Signed-off-by: Max Kellermann <mk@cm4all.com>
Reviewed-by: J. Bruce Fields <bfields@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/super.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index dada09b391c6..dab79193f641 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -977,11 +977,14 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
 	if (ctx && ctx->bsize)
 		sb->s_blocksize = nfs_block_size(ctx->bsize, &sb->s_blocksize_bits);
 
-	if (server->nfs_client->rpc_ops->version != 2) {
+	if (NFS_SB(sb)->caps & NFS_CAP_ACLS) {
 		/* The VFS shouldn't apply the umask to mode bits. We will do
 		 * so ourselves when necessary.
 		 */
 		sb->s_flags |= SB_POSIXACL;
+	}
+
+	if (server->nfs_client->rpc_ops->version != 2) {
 		sb->s_time_gran = 1;
 		sb->s_export_op = &nfs_export_ops;
 	} else
-- 
2.20.1

Re: [PATCH v3 1/4] fs/posix_acl: apply umask if superblock disables ACL support

[Christoph Hellwig]

On Tue, Apr 07, 2020 at 04:22:40PM +0200, Max Kellermann wrote:
>  
> -	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
> +	if (S_ISLNK(*mode))
>  		return 0;
>  
> +	if (!IS_POSIXACL(dir)) {
> +		*mode &= ~current_umask();
> +		return 0;
> +	}
> +

I think the first hunk is obviously correct, but I don't think we need
the second one, as the handling of the get_acl() eturn value should do
the right thing.  If you want to optimize it a bit, it might be worth to
move the !IS_POSIXACL check in get_acl to the top of the function,
before checking the cached ACL.

Re: [PATCH v3 2/4] fs/ext4/acl: apply umask if ACL support is disabled

[Christoph Hellwig]

This looks correct (modulo some minor coding style derivations),
but I think the better fix is to reuse the poix_acl_create
functionality rather than duplicating it.  Something like this:

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 8c7bbf3e566d..6cff7cc31866 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -268,33 +268,17 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 /*
  * Initialize the ACLs of a new inode. Called from ext4_new_inode.
  *
- * dir->i_mutex: down
  * inode->i_mutex: up (access to inode is still exclusive)
  */
 int
-ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
+ext4_init_acl(handle_t *handle, struct inode *inode, int type,
+		struct posix_acl *acl)
 {
-	struct posix_acl *default_acl, *acl;
-	int error;
+	int error = 0;
 
-	error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
-	if (error)
-		return error;
-
-	if (default_acl) {
-		error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
-				       default_acl, XATTR_CREATE);
-		posix_acl_release(default_acl);
-	} else {
-		inode->i_default_acl = NULL;
-	}
 	if (acl) {
-		if (!error)
-			error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
-					       acl, XATTR_CREATE);
+		error = __ext4_set_acl(handle, inode, type, acl, XATTR_CREATE);
 		posix_acl_release(acl);
-	} else {
-		inode->i_acl = NULL;
 	}
 	return error;
 }
diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
index 9b63f5416a2f..1e2927d14238 100644
--- a/fs/ext4/acl.h
+++ b/fs/ext4/acl.h
@@ -57,15 +57,16 @@ static inline int ext4_acl_count(size_t size)
 /* acl.c */
 struct posix_acl *ext4_get_acl(struct inode *inode, int type);
 int ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type);
-extern int ext4_init_acl(handle_t *, struct inode *, struct inode *);
+int ext4_init_acl(handle_t *handle, struct inode *inode, int type,
+		struct posix_acl *acl);
 
 #else  /* CONFIG_EXT4_FS_POSIX_ACL */
 #include <linux/sched.h>
 #define ext4_get_acl NULL
 #define ext4_set_acl NULL
 
-static inline int
-ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
+static inline int ext4_init_acl(handle_t *handle, struct inode *inode, int type,
+		struct posix_acl *acl)
 {
 	return 0;
 }
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index b420c9dc444d..32b03f6277c1 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1168,7 +1168,17 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	}
 
 	if (!(ei->i_flags & EXT4_EA_INODE_FL)) {
-		err = ext4_init_acl(handle, inode, dir);
+		struct posix_acl *default_acl, *acl;
+
+		cache_no_acl(inode);
+		err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+		if (err)
+			goto fail_free_drop;
+		err = ext4_init_acl(handle, inode, ACL_TYPE_DEFAULT,
+				default_acl);
+		if (err)
+			goto fail_free_drop;
+		err = ext4_init_acl(handle, inode, ACL_TYPE_ACCESS, acl);
 		if (err)
 			goto fail_free_drop;
 

Re: [PATCH v3 3/4] linux/fs.h: fix umask on NFS with CONFIG_FS_POSIX_ACL=n

[Christoph Hellwig]

> +#ifdef CONFIG_FS_POSIX_ACL
>  #define IS_POSIXACL(inode)	__IS_FLG(inode, SB_POSIXACL)
> +#else
> +#define IS_POSIXACL(inode)	0
> +#endif

Looks good, but I'd simplify this down to:

#define IS_POSIXACL(inode) \
	(IS_ENABLED(CONFIG_FS_POSIX_ACL) && __IS_FLG(inode, SB_POSIXACL))

Re: [PATCH v3 4/4] nfs/super: check NFS_CAP_ACLS instead of the NFS version

[Christoph Hellwig]

On Tue, Apr 07, 2020 at 04:22:43PM +0200, Max Kellermann wrote:
> This sets SB_POSIXACL only if ACL support is really enabled, instead
> of always setting SB_POSIXACL if the NFS protocol version
> theoretically supports ACL.
> 
> 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 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.
> 
> Signed-off-by: Max Kellermann <mk@cm4all.com>
> Reviewed-by: J. Bruce Fields <bfields@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/nfs/super.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index dada09b391c6..dab79193f641 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -977,11 +977,14 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
>  	if (ctx && ctx->bsize)
>  		sb->s_blocksize = nfs_block_size(ctx->bsize, &sb->s_blocksize_bits);
>  
> -	if (server->nfs_client->rpc_ops->version != 2) {
> +	if (NFS_SB(sb)->caps & NFS_CAP_ACLS) {
>  		/* The VFS shouldn't apply the umask to mode bits. We will do
>  		 * so ourselves when necessary.
>  		 */
>  		sb->s_flags |= SB_POSIXACL;
> +	}

Looks good, but I'd use the opportunity to also fix up the commen to be
a little less cryptic:

	/*
	 * If the server supports ACLs, the VFS shouldn't apply the umask to
	 * the mode bits as we'll do it ourselves when necessary.
	 */
	if (NFS_SB(sb)->caps & NFS_CAP_ACLS)
		sb->s_flags |= SB_POSIXACL;