[PATCH v2 1/2] nfs: fix acl memory leak of posix_acl_create()

[PATCH v2 1/2] nfs: fix acl memory leak of posix_acl_create()

[Gao Xiang]

When looking into another nfs xfstests report, I found acl and
default_acl in nfs3_proc_create() and nfs3_proc_mknod() error
paths are possibly leaked. Fix them in advance.

Fixes: 013cdf1088d7 ("nfs: use generic posix ACL infrastructure for v3 Posix ACLs")
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna.schumaker@netapp.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
no change.

 fs/nfs/nfs3proc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 5c4e23abc345..2299446b3b89 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -385,7 +385,7 @@ static void nfs3_free_createdata(struct nfs3_createdata *data)
 				break;
 
 			case NFS3_CREATE_UNCHECKED:
-				goto out;
+				goto out_release_acls;
 		}
 		nfs_fattr_init(data->res.dir_attr);
 		nfs_fattr_init(data->res.fattr);
@@ -751,7 +751,7 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *nr_arg,
 		break;
 	default:
 		status = -EINVAL;
-		goto out;
+		goto out_release_acls;
 	}
 
 	d_alias = nfs3_do_create(dir, dentry, data);
-- 
1.8.3.1

[PATCH v2 2/2] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs

[Gao Xiang]

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: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna.schumaker@netapp.com>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
v1: https://lore.kernel.org/r/1624007545-142045-1-git-send-email-hsiangkao@linux.alibaba.com 
changes since v1:
 fix a build error reported by:
  https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/GKZF3ZU6EI2WA45RV5QJ7OT6ZWMCW2BG/
  https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/S5GPZNHDQOEUCIITEQXLMIXD5IGN5QS7/
 also, some previous test results:
 https://lore.kernel.org/r/YNA03%2FY3KxHYuoLu@B-P7TQMD6M-0146.local/
 and I've also tested the nfs fstests default set w/o difference.

 fs/nfs/nfs3_fs.h  |  5 +++++
 fs/nfs/nfs3proc.c | 43 ++++++++++++++++---------------------------
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/fs/nfs/nfs3_fs.h b/fs/nfs/nfs3_fs.h
index c8a192802dda..aec803ca4afb 100644
--- a/fs/nfs/nfs3_fs.h
+++ b/fs/nfs/nfs3_fs.h
@@ -18,12 +18,17 @@ extern int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
 		struct posix_acl *dfacl);
 extern ssize_t nfs3_listxattr(struct dentry *, char *, size_t);
 extern const struct xattr_handler *nfs3_xattr_handlers[];
+#define nfs3_proc_getacl	get_acl
 #else
 static inline int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
 		struct posix_acl *dfacl)
 {
 	return 0;
 }
+static inline struct posix_acl *nfs3_proc_getacl(struct inode *inode, int type)
+{
+	return NULL;
+}
 #define nfs3_listxattr NULL
 #endif /* CONFIG_NFS_V3_ACL */
 
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 2299446b3b89..f95e75269d83 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 = nfs3_proc_getacl(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 = nfs3_proc_getacl(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 = nfs3_proc_getacl(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

Re: [PATCH v2 1/2] nfs: fix acl memory leak of posix_acl_create()

[Christoph Hellwig]

On Wed, Jun 23, 2021 at 02:38:54PM +0800, Gao Xiang wrote:
> When looking into another nfs xfstests report, I found acl and
> default_acl in nfs3_proc_create() and nfs3_proc_mknod() error
> paths are possibly leaked. Fix them in advance.

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Trond Myklebust]

On Wed, 2021-06-23 at 14:38 +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.

Hmm... Has this patch been tested with a Solaris server? Your assertion
above appears to be true for Linux servers, but this code needs to
interoperate with non-Linux draft posix acl compatible servers too.

I've already taken the other patch in this series, since that one
appears correct, and doesn't have any interoperability consequences.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Gao Xiang]

Hi Trond,

On Thu, Jun 24, 2021 at 01:13:50PM +0000, Trond Myklebust wrote:
> On Wed, 2021-06-23 at 14:38 +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.
> 
> Hmm... Has this patch been tested with a Solaris server? Your assertion
> above appears to be true for Linux servers, but this code needs to
> interoperate with non-Linux draft posix acl compatible servers too.

Sigh.. I'm not quite sure about the Solaris side since I don't have
the environment. In principle, I just think ACL inheritance should
be an underlayfs behavior rather than some nfs-specific behavior
so I assume no risk with this patch applied.

With my premature short-time glance about illumos nfs client
implementation, I don't find such setacl call  (correct me if
I'm missing...) 
https://github.com/illumos/illumos-gate/blob/9ecd05bdc59e4a1091c51ce68cce2028d5ba6fd1/usr/src/uts/common/fs/nfs/nfs3_vnops.c#L2224

(And if someone has such Solaris environment, it would be much
 helpful to check this...)

> 
> I've already taken the other patch in this series, since that one
> appears correct, and doesn't have any interoperability consequences.

Thanks all!

Thanks,
Gao Xiang

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>