[PATCH 4/6] xfs: use memdup_user()

[PATCH 4/6] xfs: use memdup_user()

[Li Zefan]

Remove open-coded memdup_user()

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/xfs/linux-2.6/xfs_ioctl.c   |   23 +++++++----------------
 fs/xfs/linux-2.6/xfs_ioctl32.c |   12 ++++--------
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index d0b4994..34eaab6 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -489,17 +489,12 @@ xfs_attrmulti_attr_set(
 	if (len > XATTR_SIZE_MAX)
 		return EINVAL;
 
-	kbuf = kmalloc(len, GFP_KERNEL);
-	if (!kbuf)
-		return ENOMEM;
-
-	if (copy_from_user(kbuf, ubuf, len))
-		goto out_kfree;
+	kbuf = memdup_user(ubuf, len);
+	if (IS_ERR(kbuf))
+		return PTR_ERR(kbuf);
 
 	error = xfs_attr_set(XFS_I(inode), name, kbuf, len, flags);
 
- out_kfree:
-	kfree(kbuf);
 	return error;
 }
 
@@ -540,20 +535,16 @@ xfs_attrmulti_by_handle(
 	if (!size || size > 16 * PAGE_SIZE)
 		goto out_dput;
 
-	error = ENOMEM;
-	ops = kmalloc(size, GFP_KERNEL);
-	if (!ops)
+	ops = memdup_user(am_hreq.ops, size);
+	if (IS_ERR(ops)) {
+		error = PTR_ERR(ops);
 		goto out_dput;
-
-	error = EFAULT;
-	if (copy_from_user(ops, am_hreq.ops, size))
-		goto out_kfree_ops;
+	}
 
 	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
 	if (!attr_name)
 		goto out_kfree_ops;
 
-
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
 		ops[i].am_error = strncpy_from_user(attr_name,
diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
index c70c4e3..0882d16 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl32.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
@@ -427,20 +427,16 @@ xfs_compat_attrmulti_by_handle(
 	if (!size || size > 16 * PAGE_SIZE)
 		goto out_dput;
 
-	error = ENOMEM;
-	ops = kmalloc(size, GFP_KERNEL);
-	if (!ops)
+	ops = memdup_user(compat_ptr(am_hreq.ops), size);
+	if (IS_ERR(ops)) {
+		error = PTR_ERR(ops);
 		goto out_dput;
-
-	error = EFAULT;
-	if (copy_from_user(ops, compat_ptr(am_hreq.ops), size))
-		goto out_kfree_ops;
+	}
 
 	attr_name = kmalloc(MAXNAMELEN, GFP_KERNEL);
 	if (!attr_name)
 		goto out_kfree_ops;
 
-
 	error = 0;
 	for (i = 0; i < am_hreq.opcount; i++) {
 		ops[i].am_error = strncpy_from_user(attr_name,
-- 
1.5.4.rc3


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: use memdup_user()

[Christoph Hellwig]

On Wed, Apr 08, 2009 at 03:08:04PM +0800, Li Zefan wrote:
> Remove open-coded memdup_user()
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/xfs/linux-2.6/xfs_ioctl.c   |   23 +++++++----------------
>  fs/xfs/linux-2.6/xfs_ioctl32.c |   12 ++++--------
>  2 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
> index d0b4994..34eaab6 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -489,17 +489,12 @@ xfs_attrmulti_attr_set(
>  	if (len > XATTR_SIZE_MAX)
>  		return EINVAL;
>  
> -	kbuf = kmalloc(len, GFP_KERNEL);
> -	if (!kbuf)
> -		return ENOMEM;
> -
> -	if (copy_from_user(kbuf, ubuf, len))
> -		goto out_kfree;
> +	kbuf = memdup_user(ubuf, len);
> +	if (IS_ERR(kbuf))
> +		return PTR_ERR(kbuf);

wouldn't NULL be a better error return for this kind of interface,
matching kmalloc?


Otherwise the patch looks good to me.

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: use memdup_user()

[Roland Dreier]

 > wouldn't NULL be a better error return for this kind of interface,
 > matching kmalloc?

I guess returning an error code from memdup_user() lets callers
distinguish between ENOMEM and EFAULT.  Not sure if that's important or
not but there probably are at least some sites that care.

 - R.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: use memdup_user()

[Felix Blyakher]

On Apr 8, 2009, at 8:22 AM, Christoph Hellwig wrote:

> On Wed, Apr 08, 2009 at 03:08:04PM +0800, Li Zefan wrote:
>> Remove open-coded memdup_user()
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>> fs/xfs/linux-2.6/xfs_ioctl.c   |   23 +++++++----------------
>> fs/xfs/linux-2.6/xfs_ioctl32.c |   12 ++++--------
>> 2 files changed, 11 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/ 
>> xfs_ioctl.c
>> index d0b4994..34eaab6 100644
>> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
>> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
>> @@ -489,17 +489,12 @@ xfs_attrmulti_attr_set(
>> 	if (len > XATTR_SIZE_MAX)
>> 		return EINVAL;
>>
>> -	kbuf = kmalloc(len, GFP_KERNEL);
>> -	if (!kbuf)
>> -		return ENOMEM;
>> -
>> -	if (copy_from_user(kbuf, ubuf, len))
>> -		goto out_kfree;
>> +	kbuf = memdup_user(ubuf, len);
>> +	if (IS_ERR(kbuf))
>> +		return PTR_ERR(kbuf);
>
> wouldn't NULL be a better error return for this kind of interface,
> matching kmalloc?
>
>
> Otherwise the patch looks good to me.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good to me too.

Reviewed-by: Felix Blyakher <felixb@sgi.com>

p.s. Replying to reply as I couldn't find the original post.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: use memdup_user()

[Li Zefan]

Roland Dreier wrote:
>  > wouldn't NULL be a better error return for this kind of interface,
>  > matching kmalloc?
> 
> I guess returning an error code from memdup_user() lets callers
> distinguish between ENOMEM and EFAULT.  Not sure if that's important or
> not but there probably are at least some sites that care.
> 

Right, and this API is simlilar to strndup_user().

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs