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@xxxxxxx> >> >> 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@xxxxxxx> >> >> --- >> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
