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

PATCH In review Krishna Kumar 2 participants 3 messages Oct 21, 2008 1 message not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Krishna Kumar

author

2008-10-21 10:51 UTC #0 lore raw

PATCH 1/1 nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

"J. Bruce Fields" <[email protected]> wrote on 10/21/2008 02:32:59 AM:

Quoted 5 lines

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.

Quoted 3 lines

- 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 <[email protected]>

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 <[email protected]>

---

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

fs/nfsd/vfs.c +7 -5

diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c
@@ -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);

A message that is not in the archive (sent off-list or not yet received): 20081021105139.13512.67162.sendpatchset-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

J. Bruce Fields

2008-10-21 22:06 UTC #2 lore raw

Quoted 8 lines · Krishna Kumar

"J. Bruce Fields" <[email protected]> wrote on 10/21/2008 02:32:59 AM:
Quoted 5 lines
Is the current code really a problem?
Possibly more interesting to look into:
- Can nfsd_getxattr actually return NULL?

Quoted 8 lines

You are right. I think it can return 0, eg any of the filesystem getxattr's.
Quoted 3 lines
- 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.

Quoted 20 lines

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);
	...
}

Quoted 5 lines

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.

Quoted 61 lines

thanks,

- KK

From: Krishna Kumar <[email protected]>

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 <[email protected]>

---

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

diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c

fs/nfsd/vfs.c +7 -6

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

2008-10-21 22:13 UTC #3 lore raw

In reply to a message not in the archive

Quoted 4 lines · Krishna Kumar

From: Krishna Kumar <[email protected]>
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?

Quoted 1 line

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.

Quoted 50 lines

Signed-off-by Krishna Kumar <[email protected]>

---

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

diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c

fs/nfsd/vfs.c +7 -6

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html