[PATCH] nfsd: Fix memory leak in nfsd_getxattr

[PATCH] nfsd: Fix memory leak in nfsd_getxattr

[Krishna Kumar]

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

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

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 fs/nfsd/vfs.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff -ruNp 2.6.27-org/fs/nfsd/vfs.c 2.6.27-new/fs/nfsd/vfs.c
--- 2.6.27-org/fs/nfsd/vfs.c	2008-10-22 14:17:23.000000000 +0530
+++ 2.6.27-new/fs/nfsd/vfs.c	2008-10-22 14:19:15.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
 

[PATCH] nfsd: Change error handling in _get_posix_acl and it's callers

[Krishna Kumar]

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

Change _get_posix_acl to return NULL on buflen == 0, and change users of
	this function to handle error cases.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 fs/nfsd/vfs.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff -ruNp 2.6.27-org/fs/nfsd/vfs.c 2.6.27-new/fs/nfsd/vfs.c
--- 2.6.27-org/fs/nfsd/vfs.c	2008-10-22 14:19:15.000000000 +0530
+++ 2.6.27-new/fs/nfsd/vfs.c	2008-10-22 14:35:16.000000000 +0530
@@ -503,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);
@@ -526,7 +524,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
 	unsigned int flags = 0;
 
 	pacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_ACCESS);
-	if (IS_ERR(pacl) && PTR_ERR(pacl) == -ENODATA)
+	if (!pacl)
 		pacl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
 	if (IS_ERR(pacl)) {
 		error = PTR_ERR(pacl);
@@ -536,9 +534,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
 
 	if (S_ISDIR(inode->i_mode)) {
 		dpacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_DEFAULT);
-		if (IS_ERR(dpacl) && PTR_ERR(dpacl) == -ENODATA)
-			dpacl = NULL;
-		else if (IS_ERR(dpacl)) {
+		if (IS_ERR(dpacl)) {
 			error = PTR_ERR(dpacl);
 			dpacl = NULL;
 			goto out;

Re: [PATCH] nfsd: Fix memory leak in nfsd_getxattr

[J. Bruce Fields]

On Wed, Oct 22, 2008 at 02:48:36PM +0530, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> Fix a memory leak in nfsd_getxattr. nfsd_getxattr should free up memory
> 	that it allocated if vfs_getxattr fails.

Thanks!  Applied to for-2.6.28.

--b.

> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
>  fs/nfsd/vfs.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff -ruNp 2.6.27-org/fs/nfsd/vfs.c 2.6.27-new/fs/nfsd/vfs.c
> --- 2.6.27-org/fs/nfsd/vfs.c	2008-10-22 14:17:23.000000000 +0530
> +++ 2.6.27-new/fs/nfsd/vfs.c	2008-10-22 14:19:15.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
>  
> --
> 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: Change error handling in _get_posix_acl and it's callers

[J. Bruce Fields]

On Wed, Oct 22, 2008 at 02:48:48PM +0530, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> Change _get_posix_acl to return NULL on buflen == 0, and change users of
> 	this function to handle error cases.

OK, the code's certainly simpler.  Makes sense.  Applied to for-2.6.29.

Hm.  Could we replace the last lines of nfsd_get_posix_acl() by a call
to _get_posix_acl() now?  The two functions are under different CONFIG_
ifdef's, so some fussing around would be required.

--b.

> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
>  fs/nfsd/vfs.c |   12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff -ruNp 2.6.27-org/fs/nfsd/vfs.c 2.6.27-new/fs/nfsd/vfs.c
> --- 2.6.27-org/fs/nfsd/vfs.c	2008-10-22 14:19:15.000000000 +0530
> +++ 2.6.27-new/fs/nfsd/vfs.c	2008-10-22 14:35:16.000000000 +0530
> @@ -503,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);
> @@ -526,7 +524,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
>  	unsigned int flags = 0;
>  
>  	pacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_ACCESS);
> -	if (IS_ERR(pacl) && PTR_ERR(pacl) == -ENODATA)
> +	if (!pacl)
>  		pacl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
>  	if (IS_ERR(pacl)) {
>  		error = PTR_ERR(pacl);
> @@ -536,9 +534,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
>  
>  	if (S_ISDIR(inode->i_mode)) {
>  		dpacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_DEFAULT);
> -		if (IS_ERR(dpacl) && PTR_ERR(dpacl) == -ENODATA)
> -			dpacl = NULL;
> -		else if (IS_ERR(dpacl)) {
> +		if (IS_ERR(dpacl)) {
>  			error = PTR_ERR(dpacl);
>  			dpacl = NULL;
>  			goto out;
> --
> 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: Change error handling in _get_posix_acl and it's callers

[J. Bruce Fields]

On Wed, Oct 22, 2008 at 02:17:53PM -0400, bfields wrote:
> On Wed, Oct 22, 2008 at 02:48:48PM +0530, Krishna Kumar wrote:
> > From: Krishna Kumar <krkumar2@in.ibm.com>
> > 
> > Change _get_posix_acl to return NULL on buflen == 0, and change users of
> > 	this function to handle error cases.
> 
> OK, the code's certainly simpler.  Makes sense.  Applied to for-2.6.29.

Whoops!  Sorry, spoke to soon--dropped.  This breaks some ACL-related
newpynfs tests:

	http://www.citi.umich.edu/projects/nfsv4/pynfs/

The assumption that nfsd_getxattr leaves *buf untouched is probably
wrong.  At the least, there's a race here--something could change
between the two calls to vfs_getxattr(), though that's not the case I'm
seeing.  The filesystem may just be giving a high estimate for the
buflen in the first call.

--b.

> >  {
> >  	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);
> > @@ -526,7 +524,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
> >  	unsigned int flags = 0;
> >  
> >  	pacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_ACCESS);
> > -	if (IS_ERR(pacl) && PTR_ERR(pacl) == -ENODATA)
> > +	if (!pacl)
> >  		pacl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
> >  	if (IS_ERR(pacl)) {
> >  		error = PTR_ERR(pacl);
> > @@ -536,9 +534,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
> >  
> >  	if (S_ISDIR(inode->i_mode)) {
> >  		dpacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_DEFAULT);
> > -		if (IS_ERR(dpacl) && PTR_ERR(dpacl) == -ENODATA)
> > -			dpacl = NULL;
> > -		else if (IS_ERR(dpacl)) {
> > +		if (IS_ERR(dpacl)) {
> >  			error = PTR_ERR(dpacl);
> >  			dpacl = NULL;
> >  			goto out;
> > --
> > 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