"J. Bruce Fields" <[email protected]> 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 <[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
--- 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);
On Tue, Oct 21, 2008 at 04:21:39PM +0530, Krishna Kumar wrote:
> "J. Bruce Fields" <[email protected]> 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 <[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
> --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 21, 2008 at 04:21:39PM +0530, Krishna Kumar wrote:
> 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?
> 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 <[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
> --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html