* [PATCH] nfs: fix acl memory leak of posix_acl_create() @ 2021-06-18 4:20 Gao Xiang 2021-06-18 9:12 ` [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs Gao Xiang 0 siblings, 1 reply; 3+ messages in thread From: Gao Xiang @ 2021-06-18 4:20 UTC (permalink / raw) To: linux-nfs Cc: LKML, Gao Xiang, Trond Myklebust, Anna Schumaker, Christoph Hellwig, Joseph Qi 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> --- If I read correctly.. 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs 2021-06-18 4:20 [PATCH] nfs: fix acl memory leak of posix_acl_create() Gao Xiang @ 2021-06-18 9:12 ` Gao Xiang 2021-06-21 6:42 ` Gao Xiang 0 siblings, 1 reply; 3+ messages in thread From: Gao Xiang @ 2021-06-18 9:12 UTC (permalink / raw) To: linux-nfs; +Cc: LKML, Gao Xiang, Trond Myklebust, Anna Schumaker, Joseph Qi 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@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> --- I didn't find the original discussion with Buck Huppmann <buchk@pobox.com> about this topic mentioned in https://lore.kernel.org/r/20050122203620.108564000@blunzn.suse.de/ 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs 2021-06-18 9:12 ` [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs Gao Xiang @ 2021-06-21 6:42 ` Gao Xiang 0 siblings, 0 replies; 3+ messages in thread From: Gao Xiang @ 2021-06-21 6:42 UTC (permalink / raw) To: linux-nfs Cc: LKML, Trond Myklebust, Anna Schumaker, Joseph Qi, Andreas Gruenbacher ( 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@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> > --- > > I didn't find the original discussion with Buck Huppmann <buchk@pobox.com> > about this topic mentioned in > https://lore.kernel.org/r/20050122203620.108564000@blunzn.suse.de/ > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-21 6:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-18 4:20 [PATCH] nfs: fix acl memory leak of posix_acl_create() Gao Xiang 2021-06-18 9:12 ` [RFC PATCH 2/1] nfs: NFSv3: fix SGID bit dropped when inheriting ACLs Gao Xiang 2021-06-21 6:42 ` Gao Xiang
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).