Re: [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs

[Gao Xiang]

( also +cc Andreas Gruenbacher )

tested NFSv3 with -g quick in my environment:
Vanilla:
Failures: generic/258 generic/294 generic/444 generic/467 generic/477 generic/531

Patched:
Failures: generic/258 generic/294 generic/467 generic/477 generic/531

It'd be much helpful to get some hints of this, am I missing something?
Many thanks!

Thanks,
Gao Xiang

On Fri, Jun 18, 2021 at 05:12:25PM +0800, Gao Xiang wrote:
> generic/444 fails with NFSv3 as shown above, "
>      QA output created by 444
>      drwxrwsr-x
>     -drwxrwsr-x
>     +drwxrwxr-x
> ", which tests "SGID inheritance with default ACLs" fs regression
> and looks after the following commits:
> 
> a3bb2d558752 ("ext4: Don't clear SGID when inheriting ACLs")
> 073931017b49 ("posix_acl: Clear SGID bit when setting file permissions")
> 
> commit 055ffbea0596 ("[PATCH] NFS: Fix handling of the umask when
> an NFSv3 default acl is present.") sets acls explicitly when
> when files are created in a directory that has a default ACL.
> However, after commit a3bb2d558752 and 073931017b49, SGID can be
> dropped if user is not member of the owning group with
> set_posix_acl() called.
> 
> Since underlayfs will handle ACL inheritance when creating
> files in a directory that has the default ACL and the umask is
> supposed to be ignored for such case. Therefore, I think no need
> to set acls explicitly (to avoid SGID bit cleared) but only apply
> client umask if the default ACL of the parent directory doesn't
> exist.
> 
> With this patch, generic/444 can pass now.
> 
> Cc: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Cc: Anna Schumaker <anna.schumaker@xxxxxxxxxx>
> Cc: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> ---
> 
> I didn't find the original discussion with Buck Huppmann <buchk@xxxxxxxxx>
> about this topic mentioned in
> https://lore.kernel.org/r/20050122203620.108564000@xxxxxxxxxxxxxx/
> 
> and it's just a rough thought about this issue...
> 
>  fs/nfs/nfs3proc.c | 43 ++++++++++++++++---------------------------
>  1 file changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index 2299446b3b89..a5676be676be 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -339,7 +339,7 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
>  nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
>  		 int flags)
>  {
> -	struct posix_acl *default_acl, *acl;
> +	struct posix_acl *pacl;
>  	struct nfs3_createdata *data;
>  	struct dentry *d_alias;
>  	int status = -ENOMEM;
> @@ -350,6 +350,10 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
>  	if (data == NULL)
>  		goto out;
>  
> +	pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> +	if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> +		sattr->ia_mode &= ~current_umask();
> +
>  	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_CREATE];
>  	data->arg.create.fh = NFS_FH(dir);
>  	data->arg.create.name = dentry->d_name.name;
> @@ -363,10 +367,6 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
>  		data->arg.create.verifier[1] = cpu_to_be32(current->pid);
>  	}
>  
> -	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> -	if (status)
> -		goto out;
> -
>  	for (;;) {
>  		d_alias = nfs3_do_create(dir, dentry, data);
>  		status = PTR_ERR_OR_ZERO(d_alias);
> @@ -416,14 +416,10 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
>  		if (status != 0)
>  			goto out_dput;
>  	}
> -
> -	status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
>  out_dput:
>  	dput(d_alias);
>  out_release_acls:
> -	posix_acl_release(acl);
> -	posix_acl_release(default_acl);
> +	posix_acl_release(pacl);
>  out:
>  	nfs3_free_createdata(data);
>  	dprintk("NFS reply create: %d\n", status);
> @@ -582,7 +578,7 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
>  static int
>  nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
>  {
> -	struct posix_acl *default_acl, *acl;
> +	struct posix_acl *pacl;
>  	struct nfs3_createdata *data;
>  	struct dentry *d_alias;
>  	int status = -ENOMEM;
> @@ -593,10 +589,9 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
>  	if (data == NULL)
>  		goto out;
>  
> -	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> -	if (status)
> -		goto out;
> -
> +	pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> +	if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> +		sattr->ia_mode &= ~current_umask();
>  	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
>  	data->arg.mkdir.fh = NFS_FH(dir);
>  	data->arg.mkdir.name = dentry->d_name.name;
> @@ -612,12 +607,9 @@ static void nfs3_proc_rename_rpc_prepare(struct rpc_task *task, struct nfs_renam
>  	if (d_alias)
>  		dentry = d_alias;
>  
> -	status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
>  	dput(d_alias);
>  out_release_acls:
> -	posix_acl_release(acl);
> -	posix_acl_release(default_acl);
> +	posix_acl_release(pacl);
>  out:
>  	nfs3_free_createdata(data);
>  	dprintk("NFS reply mkdir: %d\n", status);
> @@ -713,7 +705,7 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
>  nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
>  		dev_t rdev)
>  {
> -	struct posix_acl *default_acl, *acl;
> +	struct posix_acl *pacl;
>  	struct nfs3_createdata *data;
>  	struct dentry *d_alias;
>  	int status = -ENOMEM;
> @@ -725,9 +717,9 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
>  	if (data == NULL)
>  		goto out;
>  
> -	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
> -	if (status)
> -		goto out;
> +	pacl = get_acl(dir, ACL_TYPE_DEFAULT);
> +	if (!pacl || pacl == ERR_PTR(-EOPNOTSUPP))
> +		sattr->ia_mode &= ~current_umask();
>  
>  	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKNOD];
>  	data->arg.mknod.fh = NFS_FH(dir);
> @@ -762,12 +754,9 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
>  	if (d_alias)
>  		dentry = d_alias;
>  
> -	status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> -
>  	dput(d_alias);
>  out_release_acls:
> -	posix_acl_release(acl);
> -	posix_acl_release(default_acl);
> +	posix_acl_release(pacl);
>  out:
>  	nfs3_free_createdata(data);
>  	dprintk("NFS reply mknod: %d\n", status);
> -- 
> 1.8.3.1