[PATCH 14/27] drivers/scsi: Use memdup_user

[PATCH 14/27] drivers/scsi: Use memdup_user

[Julia Lawall]

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);
 
 		len = iov_length(iov, iov_count);
 		if (hp->dxfer_len < len) {

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[James Bottomley]

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.

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

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[Boaz Harrosh]

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

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[James Bottomley]

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

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[Boaz Harrosh]

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

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[James Bottomley]

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

Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[Andrew Morton]

On Tue, 25 May 2010 09:58:33 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> 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.

kerrrrapp!!  There's always value in replacing open-coded straggles
with calls to well-understood, well-documented, well-tested library
code.

>  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

Hello?  This is Linux.  We're continuously filtering through old code,
improving and updating it.  There's nothing which exempts
./drivers/scsi/ from our reasons for doing this.

> (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.

copy_from_user() can take a fault, take a zillion locks, perform
GFP_HIGHUSER allocations, enter page reclaim, enter filesystems, etc. 
View a pagefault as a syscall with a different calling convention -
it's that high level.

If this driver if correct in potentially taking a pagefault at this
place then the patch is correct and desirable.  Otherwise, well, the
driver already has problem and the patch did nothing to increase them.

And, of course, Julia's "no value" work caused us to identify those
problems.