On Mon, 2020-04-20 at 13:43 +0800, Xiyu Yang wrote:
> nfs3_set_acl() invokes get_acl(), which returns a local reference of
> the
> posix_acl object to "alloc" with increased refcount.
>
> When nfs3_set_acl() returns or a new object is assigned to
> "alloc", the
> original local reference of "alloc" becomes invalid, so the refcount
> should be decreased to keep refcount balanced.
>
> The reference counting issue happens in one path of nfs3_set_acl().
> When
> "acl" equals to NULL but "alloc" is not NULL, the function forgets to
> decrease the refcnt increased by get_acl() and causes a refcnt leak.
>
> Fix this issue by calling posix_acl_release() on this path when
> "alloc"
> is not NULL.
>
> Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs")
> Signed-off-by: Xiyu Yang <[email protected]>
> Signed-off-by: Xin Tan <[email protected]>
> ---
> fs/nfs/nfs3acl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index c5c3fc6e6c60..b5c41bcca8cf 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -274,6 +274,8 @@ int nfs3_set_acl(struct inode *inode, struct
> posix_acl *acl, int type)
> }
>
> if (acl == NULL) {
> + if (alloc)
> + posix_acl_release(alloc);
This will result in 'dfacl' being freed while it is still in use, so
can't be correct either.
> alloc = acl = posix_acl_from_mode(inode->i_mode,
> GFP_KERNEL);
> if (IS_ERR(alloc))
> goto fail;
I don't really see any alternative to adding a 'dfalloc' to track the
allocated dfacl separately.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust <[email protected]>:
> I don't really see any alternative to adding a 'dfalloc' to track the
> allocated dfacl separately.
Something like the attached patch should work as well.
Thanks,
Andreas
---
fs/nfs/nfs3acl.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index c5c3fc6e6c60..f1581f11c220 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type)
{
- struct posix_acl *alloc = NULL, *dfacl = NULL;
+ struct posix_acl *orig = acl, *dfacl = NULL;
int status;
if (S_ISDIR(inode->i_mode)) {
switch(type) {
case ACL_TYPE_ACCESS:
- alloc = dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
- if (IS_ERR(alloc))
- goto fail;
+ dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
+ status = PTR_ERR(dfacl);
+ if (IS_ERR(dfacl))
+ goto out;
break;
case ACL_TYPE_DEFAULT:
dfacl = acl;
- alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
- if (IS_ERR(alloc))
- goto fail;
+ acl = get_acl(inode, ACL_TYPE_ACCESS);
+ status = PTR_ERR(acl);
+ if (IS_ERR(acl))
+ goto out;
break;
}
}
if (acl == NULL) {
- alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
- if (IS_ERR(alloc))
- goto fail;
+ acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
+ status = PTR_ERR(acl);
+ if (IS_ERR(acl))
+ goto out;
}
status = __nfs3_proc_setacls(inode, acl, dfacl);
- posix_acl_release(alloc);
+out:
+ if (acl != orig)
+ posix_acl_release(acl);
+ if (dfacl != orig)
+ posix_acl_release(dfacl);
return status;
-
-fail:
- return PTR_ERR(alloc);
}
const struct xattr_handler *nfs3_xattr_handlers[] = {
base-commit: ae83d0b416db002fe95601e7f97f64b59514d936
--
2.25.3
On Mon, 2020-04-20 at 14:51 +0200, Andreas Gruenbacher wrote:
> Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust <
> [email protected]>:
> > I don't really see any alternative to adding a 'dfalloc' to track
> > the
> > allocated dfacl separately.
>
> Something like the attached patch should work as well.
>
> Thanks,
> Andreas
>
> ---
> fs/nfs/nfs3acl.c | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index c5c3fc6e6c60..f1581f11c220 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode,
> struct posix_acl *acl,
>
> int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int
> type)
> {
> - struct posix_acl *alloc = NULL, *dfacl = NULL;
> + struct posix_acl *orig = acl, *dfacl = NULL;
> int status;
>
> if (S_ISDIR(inode->i_mode)) {
> switch(type) {
> case ACL_TYPE_ACCESS:
> - alloc = dfacl = get_acl(inode,
> ACL_TYPE_DEFAULT);
> - if (IS_ERR(alloc))
> - goto fail;
> + dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
> + status = PTR_ERR(dfacl);
> + if (IS_ERR(dfacl))
> + goto out;
> break;
>
> case ACL_TYPE_DEFAULT:
> dfacl = acl;
> - alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
> - if (IS_ERR(alloc))
> - goto fail;
> + acl = get_acl(inode, ACL_TYPE_ACCESS);
> + status = PTR_ERR(acl);
> + if (IS_ERR(acl))
> + goto out;
> break;
> }
> }
>
> if (acl == NULL) {
> - alloc = acl = posix_acl_from_mode(inode->i_mode,
> GFP_KERNEL);
> - if (IS_ERR(alloc))
> - goto fail;
> + acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
> + status = PTR_ERR(acl);
> + if (IS_ERR(acl))
> + goto out;
> }
> status = __nfs3_proc_setacls(inode, acl, dfacl);
> - posix_acl_release(alloc);
> +out:
> + if (acl != orig)
> + posix_acl_release(acl);
> + if (dfacl != orig)
> + posix_acl_release(dfacl);
> return status;
> -
> -fail:
> - return PTR_ERR(alloc);
> }
>
> const struct xattr_handler *nfs3_xattr_handlers[] = {
>
> base-commit: ae83d0b416db002fe95601e7f97f64b59514d936
Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
triggers, shouldn't it? ????
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust <[email protected]> wrote:
> Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
> triggers, shouldn't it?
Yes, checks missings. But you get the idea.
Thanks,
Andreas
On Mon, 2020-04-20 at 15:04 +0200, Andreas Gruenbacher wrote:
> On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust <
> [email protected]> wrote:
> > Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
> > triggers, shouldn't it?
>
> Yes, checks missings. But you get the idea.
>
Fair enough ????
Can I expect an official patch from either you or the original
submitters?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]