[PATCH] nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

[PATCH] nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

[Krishna Kumar]

"J. Bruce Fields" <bfields@fieldses.org> wrote on 10/21/2008 02:32:59 AM:

> Is the current code really a problem?
> 
> Possibly more interesting to look into:
> 
>    - Can nfsd_getxattr actually return NULL?

You are right. I think it can return 0, eg any of the filesystem getxattr's.

>    - Why do we need to return ENODATA in that case?
>      (nfsd_get_posix_acl, for example, seems to just end up
>      returning -EINVAL in that case.)

Did you mean NULL rather than EINVAL (posix_acl_from_xattr returns NULL before
EINVAL)?

Another thing that seems slightly wrong. Both of the following functions:
nfsd_get_posix_acl()
{
	...
	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
	if (size < 0)
		return ERR_PTR(size);
	...
}
_get_posix_acl(struct dentry *dentry, char *key)
{
	...
	buflen = nfsd_getxattr(dentry, key, &buf);
	if (!buflen)
		buflen = -ENODATA;
	if (buflen <= 0)
		return ERR_PTR(buflen);
	...
}
call nfsd_getxattr which can allocate memory but still return < 0. Function
allocates buf and calls vfs_getxattr() which fails and leaks memory. The best
place to fix this is in nfsd_getxattr in case vfs_getxattr() returns < 0

Does that sound correct? Patch for this follows...

thanks,

- KK

From: Krishna Kumar <krkumar2@in.ibm.com>

1. Fix a memory leak in nfsd_getxattr. nfsd_getxattr should free up memory
	that it allocated if vfs_getxattr fails.

2. Change _get_posix_acl to return NULL on buflen == 0.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---

 fs/nfsd/vfs.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c
--- org/fs/nfsd/vfs.c	2008-10-21 16:05:41.000000000 +0530
+++ new/fs/nfsd/vfs.c	2008-10-21 15:57:30.000000000 +0530
@@ -411,6 +411,7 @@ out_nfserr:
 static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
 {
 	ssize_t buflen;
+	ssize_t ret;
 
 	buflen = vfs_getxattr(dentry, key, NULL, 0);
 	if (buflen <= 0)
@@ -420,7 +421,10 @@ static ssize_t nfsd_getxattr(struct dent
 	if (!*buf)
 		return -ENOMEM;
 
-	return vfs_getxattr(dentry, key, *buf, buflen);
+	ret = vfs_getxattr(dentry, key, *buf, buflen);
+	if (ret < 0)
+		kfree(*buf);
+	return ret;
 }
 #endif
 
@@ -499,13 +503,11 @@ static struct posix_acl *
 _get_posix_acl(struct dentry *dentry, char *key)
 {
 	void *buf = NULL;
-	struct posix_acl *pacl = NULL;
+	struct posix_acl *pacl;
 	int buflen;
 
 	buflen = nfsd_getxattr(dentry, key, &buf);
-	if (!buflen)
-		buflen = -ENODATA;
-	if (buflen <= 0)
+	if (buflen < 0)
 		return ERR_PTR(buflen);
 
 	pacl = posix_acl_from_xattr(buf, buflen);

Re: [PATCH] nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

[J. Bruce Fields]

On Tue, Oct 21, 2008 at 04:21:39PM +0530, Krishna Kumar wrote:
> "J. Bruce Fields" <bfields@fieldses.org> wrote on 10/21/2008 02:32:59 AM:
> 
> > Is the current code really a problem?
> > 
> > Possibly more interesting to look into:
> > 
> >    - Can nfsd_getxattr actually return NULL?
> 
> You are right. I think it can return 0, eg any of the filesystem getxattr's.
> 
> >    - Why do we need to return ENODATA in that case?
> >      (nfsd_get_posix_acl, for example, seems to just end up
> >      returning -EINVAL in that case.)
> 
> Did you mean NULL rather than EINVAL (posix_acl_from_xattr returns NULL before
> EINVAL)?

Whoops, you're right--I missed that.

> 
> Another thing that seems slightly wrong. Both of the following functions:
> nfsd_get_posix_acl()
> {
> 	...
> 	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
> 	if (size < 0)
> 		return ERR_PTR(size);
> 	...
> }
> _get_posix_acl(struct dentry *dentry, char *key)
> {
> 	...
> 	buflen = nfsd_getxattr(dentry, key, &buf);
> 	if (!buflen)
> 		buflen = -ENODATA;
> 	if (buflen <= 0)
> 		return ERR_PTR(buflen);
> 	...
> }
> call nfsd_getxattr which can allocate memory but still return < 0. Function
> allocates buf and calls vfs_getxattr() which fails and leaks memory. The best
> place to fix this is in nfsd_getxattr in case vfs_getxattr() returns < 0
> 
> Does that sound correct? Patch for this follows...

Yes, that sounds correct.  I'll take a look at the patch.

--b.

> 
> thanks,
> 
> - KK
> 
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> 1. Fix a memory leak in nfsd_getxattr. nfsd_getxattr should free up memory
> 	that it allocated if vfs_getxattr fails.
> 
> 2. Change _get_posix_acl to return NULL on buflen == 0.
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> 
>  fs/nfsd/vfs.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c
> --- org/fs/nfsd/vfs.c	2008-10-21 16:05:41.000000000 +0530
> +++ new/fs/nfsd/vfs.c	2008-10-21 15:57:30.000000000 +0530
> @@ -411,6 +411,7 @@ out_nfserr:
>  static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
>  {
>  	ssize_t buflen;
> +	ssize_t ret;
>  
>  	buflen = vfs_getxattr(dentry, key, NULL, 0);
>  	if (buflen <= 0)
> @@ -420,7 +421,10 @@ static ssize_t nfsd_getxattr(struct dent
>  	if (!*buf)
>  		return -ENOMEM;
>  
> -	return vfs_getxattr(dentry, key, *buf, buflen);
> +	ret = vfs_getxattr(dentry, key, *buf, buflen);
> +	if (ret < 0)
> +		kfree(*buf);
> +	return ret;
>  }
>  #endif
>  
> @@ -499,13 +503,11 @@ static struct posix_acl *
>  _get_posix_acl(struct dentry *dentry, char *key)
>  {
>  	void *buf = NULL;
> -	struct posix_acl *pacl = NULL;
> +	struct posix_acl *pacl;
>  	int buflen;
>  
>  	buflen = nfsd_getxattr(dentry, key, &buf);
> -	if (!buflen)
> -		buflen = -ENODATA;
> -	if (buflen <= 0)
> +	if (buflen < 0)
>  		return ERR_PTR(buflen);
>  
>  	pacl = posix_acl_from_xattr(buf, buflen);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

[J. Bruce Fields]

On Tue, Oct 21, 2008 at 04:21:39PM +0530, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> 1. Fix a memory leak in nfsd_getxattr. nfsd_getxattr should free up memory
> 	that it allocated if vfs_getxattr fails.

This part looks fine, thanks.  But could you submit it as a separate
patch?

> 2. Change _get_posix_acl to return NULL on buflen == 0.

If we do this, then the callers of _get_posix_acl() also need to be
fixed.  In any case, make it a second patch.

--b.

> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> 
>  fs/nfsd/vfs.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c
> --- org/fs/nfsd/vfs.c	2008-10-21 16:05:41.000000000 +0530
> +++ new/fs/nfsd/vfs.c	2008-10-21 15:57:30.000000000 +0530
> @@ -411,6 +411,7 @@ out_nfserr:
>  static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
>  {
>  	ssize_t buflen;
> +	ssize_t ret;
>  
>  	buflen = vfs_getxattr(dentry, key, NULL, 0);
>  	if (buflen <= 0)
> @@ -420,7 +421,10 @@ static ssize_t nfsd_getxattr(struct dent
>  	if (!*buf)
>  		return -ENOMEM;
>  
> -	return vfs_getxattr(dentry, key, *buf, buflen);
> +	ret = vfs_getxattr(dentry, key, *buf, buflen);
> +	if (ret < 0)
> +		kfree(*buf);
> +	return ret;
>  }
>  #endif
>  
> @@ -499,13 +503,11 @@ static struct posix_acl *
>  _get_posix_acl(struct dentry *dentry, char *key)
>  {
>  	void *buf = NULL;
> -	struct posix_acl *pacl = NULL;
> +	struct posix_acl *pacl;
>  	int buflen;
>  
>  	buflen = nfsd_getxattr(dentry, key, &buf);
> -	if (!buflen)
> -		buflen = -ENODATA;
> -	if (buflen <= 0)
> +	if (buflen < 0)
>  		return ERR_PTR(buflen);
>  
>  	pacl = posix_acl_from_xattr(buf, buflen);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html