lkml.org 
[lkml]   [2010]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 14/27] drivers/scsi: Use memdup_user
On 05/22/2010 05:24 PM, James Bottomley wrote:
> On Sat, 2010-05-22 at 10:22 +0200, Julia Lawall wrote:
>> From: Julia Lawall <julia@diku.dk>
>>
>> Use memdup_user when user data is immediately copied into the
>> allocated region.
>>
>> The semantic patch that makes this change is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression from,to,size,flag;
>> position p;
>> identifier l1,l2;
>> @@
>>
>> - to = \(kmalloc@p\|kzalloc@p\)(size,flag);
>> + to = memdup_user(from,size);
>> if (
>> - to==NULL
>> + IS_ERR(to)
>> || ...) {
>> <+... when != goto l1;
>> - -ENOMEM
>> + PTR_ERR(to)
>> ...+>
>> }
>> - if (copy_from_user(to, from, size) != 0) {
>> - <+... when != goto l2;
>> - -EFAULT
>> - ...+>
>> - }
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>
>> ---
>> drivers/scsi/sg.c | 11 +++--------
>> 1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index ef752b2..277ace6 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -1676,14 +1676,9 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
>> int len, size = sizeof(struct sg_iovec) * iov_count;
>> struct iovec *iov;
>>
>> - iov = kmalloc(size, GFP_ATOMIC);
>> - if (!iov)
>> - return -ENOMEM;
>> -
>> - if (copy_from_user(iov, hp->dxferp, size)) {
>> - kfree(iov);
>> - return -EFAULT;
>> - }
>> + iov = memdup_user(hp->dxferp, size);
>> + if (IS_ERR(iov))
>> + return PTR_ERR(iov);
>
> This type of transformation has really no value at all. The code you're
> proposing to replace is already correct. I'm fairly ambivalent on
> patterned APIs anyway but I accept they're useful way to prevent new
> code getting it wrong. However, it's completely bogus to force
> replacement of correctly functioning code throughout the kernel (unless
> you're going to argue that everyone who tries to copy from user into a
> kmalloc space does a cut and paste from sg?)
>
> Of infinitely greater service would be finding any places where the
> pattern is being incorrectly used.
>

It looks like it is not done 100% kosher and calling memdup_user should
be better.

- For 1 memdup_user does a GFP_KERNEL with a comment on how copy_from_user
would eventually sleep, so what's the point of GFP_ATOMIC?
If this is by design then it surly calls for a comment that explains.
(I would like to know)

- 2nd kmalloc_track_caller is called which is more appropriate here by
a small margin, No?

Just my $0.017
Boaz

> So, if Doug wants to take this as a prettify of sg, I'm happy, but if
> not, I don't really want to see this again.
>
> Thanks,
>
> James
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html



\
 
 \ /
  Last update: 2010-05-23 17:39    [W:0.046 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site