drivers/scsi: Use memdup_user
in [Kernel]
|
Prev: [PATCH] NUC900: add maintainer entries for Wan ZongShun.
Next: input: mt: Document the MT event slot protocol (rev3)
From: James Bottomley on 22 May 2010 10:30 On Sat, 2010-05-22 at 10:22 +0200, Julia Lawall wrote: > From: Julia Lawall <julia(a)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(a)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. 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-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Boaz Harrosh on 23 May 2010 11:40 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(a)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(a)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(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: James Bottomley on 23 May 2010 12:30 On Sun, 2010-05-23 at 18:36 +0300, Boaz Harrosh wrote: > > 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? Well, since you've written a storage driver, I really hope that question is rhetorical ... The reason for using GFP_ATOMIC from user context in storage drivers is to avoid writeout deadlock: you don't want to trigger a swap write while you potentially occupy the writeout path. In all older drivers this had to be GFP_ATOMIC because GFP_NOIO wasn't around. This also illustrates the problem with design patterns: The idea that if you have user context, you must be able to kmalloc GFP_KERNEL seems logical to the people who wrote the pattern, but actually it's potentially incorrect for storage. Now in the particular case of sg, I don't believe we'll ever want to swap over sg (famous last words, of course), so in this instance we probably could get away with using GFP_KERNEL ... but since it's following the storage pattern, GFP_ATOMIC (or GFP_NOIO) is correct. Does osd need auditing for this problem (or would no-one ever do swap over osd)? > If this is by design then it surly calls for a comment that explains. > (I would like to know) This pattern occurs many times in storage ... documenting it at every callsite would be a huge waste. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Boaz Harrosh on 25 May 2010 10:20 On 05/23/2010 07:22 PM, James Bottomley wrote: > On Sun, 2010-05-23 at 18:36 +0300, Boaz Harrosh wrote: >>> 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? > > Well, since you've written a storage driver, I really hope that question > is rhetorical ... > Not really I do mean it in this case. It is not a storage-driver code path. Is sg a storage-driver. I was not aware anyone used it for one. > The reason for using GFP_ATOMIC from user context in storage drivers is > to avoid writeout deadlock: you don't want to trigger a swap write > while you potentially occupy the writeout path. In all older drivers > this had to be GFP_ATOMIC because GFP_NOIO wasn't around. > I confess my total swap ignorance. I was under the impression that swap needs a block device. OK you could do a user-mode filesystem over sg, under loop device. Does swap work over loop device=>ext2 for example? Or is there a way to directly swap over a filesystem (Not block based)? > This also illustrates the problem with design patterns: The idea that > if you have user context, you must be able to kmalloc GFP_KERNEL seems > logical to the people who wrote the pattern, but actually it's > potentially incorrect for storage. > Again I confess my ignorance but I thought that copy_from_user can sleep because of a page_fault which will invoke the memory allocation subsystem anyway, so the out-of-memory condition will apply to copy_from_user as well. But I don't have a clue, actually, why the copy_from_user might sleep. > Now in the particular case of sg, I don't believe we'll ever want to > swap over sg (famous last words, of course), so in this instance we > probably could get away with using GFP_KERNEL ... but since it's > following the storage pattern, GFP_ATOMIC (or GFP_NOIO) is correct. > Again I never considered sg as storage-driver. It is not a block-device and is only used from user-mode that has it's out-of-memory-sleep problems without sg. > Does osd need auditing for this problem (or would no-one ever do swap > over osd)? > libosd's members all receive a gfp_t parameter and let the caller determine the sleep policy. For example in osdblk allocations are done with GFP_ATOMIC. At the exofs level everything is GFP_KERNEL because that is one of the rules of VFS API. So osdblk should theoretically be able to swap. That said, in current kernel an iscsi device cannot be used for swap because of NET. There was grate progress done to try and swap over nfs which improved iscsi as well, but it was never seen all the way through. >> If this is by design then it surly calls for a comment that explains. >> (I would like to know) > > This pattern occurs many times in storage ... documenting it at every > callsite would be a huge waste. > So if this is serious, them maybe call copy_from_user_inatomic to match the GFP_ATOMIC allocation? > James > Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: James Bottomley on 25 May 2010 11:00
On Tue, 2010-05-25 at 17:15 +0300, Boaz Harrosh wrote: > On 05/23/2010 07:22 PM, James Bottomley wrote: > > On Sun, 2010-05-23 at 18:36 +0300, Boaz Harrosh wrote: > >>> 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? > > > > Well, since you've written a storage driver, I really hope that question > > is rhetorical ... > > > > Not really I do mean it in this case. It is not a storage-driver code > path. Is sg a storage-driver. I was not aware anyone used it for one. by storage driver, I mean anything in the writeout path ... this includes ULDs, of which sg is one. > > The reason for using GFP_ATOMIC from user context in storage drivers is > > to avoid writeout deadlock: you don't want to trigger a swap write > > while you potentially occupy the writeout path. In all older drivers > > this had to be GFP_ATOMIC because GFP_NOIO wasn't around. > > > > I confess my total swap ignorance. I was under the impression that > swap needs a block device. OK you could do a user-mode filesystem > over sg, under loop device. Does swap work over loop device=>ext2 > for example? Or is there a way to directly swap over a filesystem > (Not block based)? Well, swap can function above block devices or files. However, it's not just swap, it's also triggerable via mmap (you do file backed mmap of a large area then have a process that joyfully dirties lots of it) ... and that's what can make it such a difficult problem. Basically you end up with a system with mostly dirty memory that it has to clean via writeout (either to swap or by flushing dirty pages). The writeout path that these pages go down cannot ever wait on dirty pages being flushed (which is what a sleeping kmalloc does), because that can end up as a deadlock. > > This also illustrates the problem with design patterns: The idea that > > if you have user context, you must be able to kmalloc GFP_KERNEL seems > > logical to the people who wrote the pattern, but actually it's > > potentially incorrect for storage. > > > > Again I confess my ignorance but I thought that copy_from_user can sleep > because of a page_fault which will invoke the memory allocation subsystem > anyway, so the out-of-memory condition will apply to copy_from_user as > well. But I don't have a clue, actually, why the copy_from_user might > sleep. It can ... my point was that the assumption that just because the user copy can sleep on a page fault doesn't mean that the kmalloc can (although it seems obvious that it should, and that's the trap). > > Now in the particular case of sg, I don't believe we'll ever want to > > swap over sg (famous last words, of course), so in this instance we > > probably could get away with using GFP_KERNEL ... but since it's > > following the storage pattern, GFP_ATOMIC (or GFP_NOIO) is correct. > > > > Again I never considered sg as storage-driver. It is not a block-device > and is only used from user-mode that has it's out-of-memory-sleep problems > without sg. > > > Does osd need auditing for this problem (or would no-one ever do swap > > over osd)? > > > > libosd's members all receive a gfp_t parameter and let the caller > determine the sleep policy. For example in osdblk allocations are > done with GFP_ATOMIC. At the exofs level everything is GFP_KERNEL > because that is one of the rules of VFS API. So osdblk should > theoretically be able to swap. if exofs is GFP_KERNEL, it can get into the same issue if users can mmap large memory areas over exofs files ... or if exofs supports swap files. > That said, in current kernel an iscsi device cannot be used for swap > because of NET. There was grate progress done to try and swap over > nfs which improved iscsi as well, but it was never seen all the way > through. > > >> If this is by design then it surly calls for a comment that explains. > >> (I would like to know) > > > > This pattern occurs many times in storage ... documenting it at every > > callsite would be a huge waste. > > > > So if this is serious, them maybe call copy_from_user_inatomic to match > the GFP_ATOMIC allocation? Possibly ... although this is a parameter area, the chances of losing it between setting it up and calling sg_write are pretty small. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |