nfsd: Fix memory leak in nfsd_getxattr

PATCH In review Krishna Kumar 2 participants 5 messages Oct 22, 2008 2 messages not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Krishna Kumar

author

2008-10-22 09:18 UTC #0 lore raw

PATCH 1/1 nfsd: Fix memory leak in nfsd_getxattr

From: Krishna Kumar <[email protected]>

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

---
 fs/nfsd/vfs.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/nfsd/vfs.c +5 -1

diff -ruNp 2.6.27-org/fs/nfsd/vfs.c 2.6.27-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

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

Krishna Kumar

author

2008-10-22 09:18 UTC #2 lore raw

PATCH 1/1 nfsd: Change error handling in _get_posix_acl and it's callers

From: Krishna Kumar <[email protected]>

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

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

fs/nfsd/vfs.c +4 -8

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

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

J. Bruce Fields

2008-10-22 18:17 UTC #4 lore raw

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

Quoted 4 lines · Krishna Kumar

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

Quoted 48 lines

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

---
 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

fs/nfsd/vfs.c +4 -9

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

J. Bruce Fields

2008-10-22 20:12 UTC #5 lore raw

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

Quoted 7 lines · bfields

Quoted 4 lines · Krishna Kumar
From: Krishna Kumar <[email protected]>
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.

Quoted 38 lines

{

fs/nfsd/vfs.c +4 -9

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

J. Bruce Fields

2008-10-22 18:12 UTC #6 lore raw

In reply to a message not in the archive

Quoted 4 lines · Krishna Kumar

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

Quoted 33 lines

Signed-off-by Krishna Kumar <[email protected]>
---
 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
fs/nfsd/vfs.c +5 -2
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html