[PATCH] usb: use memdup_user()

[PATCH] usb: use memdup_user()

[Li Hong]

Replace a combination call of kmalloc() and copy_from_user() with memdup_user().

Signed-off-by: Li Hong <lihong.hi@gmail.com>
---
 drivers/usb/class/cdc-wdm.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 0fe4345..326ecf3 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -324,18 +324,11 @@ static ssize_t wdm_write
                goto out;
        }

-       desc->outbuf = buf = kmalloc(count, GFP_KERNEL);
-       if (!buf) {
-               rv = -ENOMEM;
+       desc->outbuf = buf = memdup_user(buffer, count);
+       if (IS_ERR(buf)) {
+               rv = PTR_ERR(buf);
                goto out;
-       }
-
-       r = copy_from_user(buf, buffer, count);
-       if (r > 0) {
-               kfree(buf);
-               rv = -EFAULT;
-               goto out;
-       }
+       }

        req = desc->orq;
        usb_fill_control_urb(
-- 
1.6.2.GIT

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Sonntag, 3. Mai 2009 18:00:56 schrieb Li Hong:
> Replace a combination call of kmalloc() and copy_from_user() with
> memdup_user().

I must veto this. We lose the obvious GFP_KERNEL here. If we have
to change this for some reason we are screwed.

	Sorry
		Oliver

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Sonntag, 3. Mai 2009 18:00:56 schrieb Li Hong:
> Replace a combination call of kmalloc() and copy_from_user() with
> memdup_user().

To be precise. copy_*_user() of course implies that you can do
GFP_KERNEL. But the point is one of design. Right now I can change
locking and if that means that one must use GFP_NOIO, which is common
in usb due to the storage driver, we can find out by simply grepping and
move the user space copy outside of the loop.

If you make that change we must grep against a second and rare
key.

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[Li Hong]

The key is: is there any need or probability to use GFP_NOIO instead
of GFP_KERNEL _here_? It also confuses me to move the user space copy
outside of the loop.

IMHO, this is just a simple cleanup of the code.

Regards,
Hong

2009/5/3 Oliver Neukum <oliver@neukum.org>:
> Am Sonntag, 3. Mai 2009 18:00:56 schrieb Li Hong:
>> Replace a combination call of kmalloc() and copy_from_user() with
>> memdup_user().
>
> To be precise. copy_*_user() of course implies that you can do
> GFP_KERNEL. But the point is one of design. Right now I can change
> locking and if that means that one must use GFP_NOIO, which is common
> in usb due to the storage driver, we can find out by simply grepping and
> move the user space copy outside of the loop.
>
> If you make that change we must grep against a second and rare
> key.
>
>        Regards
>                Oliver
>
>

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Montag, 4. Mai 2009 05:38:33 schrieb Li Hong:
> The key is: is there any need or probability to use GFP_NOIO instead
> of GFP_KERNEL _here_? It also confuses me to move the user space copy
> outside of the loop.

No. To make it plain. To me any use of memdup_user() in USB code
is a bad idea. I don't want to have to think about a new primitive.

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[David Brownell]

On Sunday 03 May 2009, Oliver Neukum wrote:
> No. To make it plain. To me any use of memdup_user() in USB code
> is a bad idea. I don't want to have to think about a new primitive.

Unless it's incorrect to use that, I have to say that it
makes more sense to use that utility than recreate it by
open-coding...

Re: [PATCH] usb: use memdup_user()

[Pekka Enberg]

Hi!

On Sunday 03 May 2009, Oliver Neukum wrote:
>> No. To make it plain. To me any use of memdup_user() in USB code
>> is a bad idea. I don't want to have to think about a new primitive.

On Mon, May 4, 2009 at 10:02 AM, David Brownell <david-b@pacbell.net> wrote:
> Unless it's incorrect to use that, I have to say that it
> makes more sense to use that utility than recreate it by
> open-coding...

Yup, and I don't really see how anyone can avoid "thinking about a new
primitive" anyway. We have it in the kernel now and surely it will
appear under drivers/usb/ sooner or later...

                        Pekka

Re: [PATCH] usb: use memdup_user()

[Greg KH]

On Mon, May 04, 2009 at 10:41:51AM +0300, Pekka Enberg wrote:
> Hi!
> 
> On Sunday 03 May 2009, Oliver Neukum wrote:
> >> No. To make it plain. To me any use of memdup_user() in USB code
> >> is a bad idea. I don't want to have to think about a new primitive.
> 
> On Mon, May 4, 2009 at 10:02 AM, David Brownell <david-b@pacbell.net> wrote:
> > Unless it's incorrect to use that, I have to say that it
> > makes more sense to use that utility than recreate it by
> > open-coding...
> 
> Yup, and I don't really see how anyone can avoid "thinking about a new
> primitive" anyway. We have it in the kernel now and surely it will
> appear under drivers/usb/ sooner or later...

Well, how about passing the GPF flags down to memdup_user() so that we
can use it in the usb subsystem, and Oliver's complaint will be resolve?

thanks,

greg k-h

Re: [PATCH] usb: use memdup_user()

[Li Hong]

2009/5/4 Greg KH <greg@kroah.com>:
> On Mon, May 04, 2009 at 10:41:51AM +0300, Pekka Enberg wrote:
>> Hi!
>>
>> On Sunday 03 May 2009, Oliver Neukum wrote:
>> >> No. To make it plain. To me any use of memdup_user() in USB code
>> >> is a bad idea. I don't want to have to think about a new primitive.
>>
>> On Mon, May 4, 2009 at 10:02 AM, David Brownell <david-b@pacbell.net> wrote:
>> > Unless it's incorrect to use that, I have to say that it
>> > makes more sense to use that utility than recreate it by
>> > open-coding...
>>
>> Yup, and I don't really see how anyone can avoid "thinking about a new
>> primitive" anyway. We have it in the kernel now and surely it will
>> appear under drivers/usb/ sooner or later...
>
> Well, how about passing the GPF flags down to memdup_user() so that we
> can use it in the usb subsystem, and Oliver's complaint will be resolve?
>
> thanks,
>
> greg k-h
>

No, the debate is not here. As the comment in memdup_user() says:

        /*
         * Always use GFP_KERNEL, since copy_from_user() can sleep and
         * cause pagefault, which makes it pointless to use GFP_NOFS
         * or GFP_ATOMIC.
         */

So it is pointless to add a GFP flag to memdup_user().

I guess what Oliver insists is that the involvement of memdup_user() erases
the explicit call of kmalloc(). However, doesn't every function wrap
some details?

Regards,
Hong

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Montag, 4. Mai 2009 16:53:48 schrieb Greg KH:
> > On Mon, May 4, 2009 at 10:02 AM, David Brownell <david-b@pacbell.net> 
wrote:
> > > Unless it's incorrect to use that, I have to say that it
> > > makes more sense to use that utility than recreate it by
> > > open-coding...
> >
> > Yup, and I don't really see how anyone can avoid "thinking about a new
> > primitive" anyway. We have it in the kernel now and surely it will
> > appear under drivers/usb/ sooner or later...
>
> Well, how about passing the GPF flags down to memdup_user() so that we
> can use it in the usb subsystem, and Oliver's complaint will be resolve?

It would always be GFP_KERNEL. Unless you can use GFP_KERNEL you must
not use copy_to/from_user. It would not make sense for all other users of that
API. I don't think that the API as such is bad, just that USB needs extra care
with memory allocations (and user memory access)

We are limited to GFP_NOIO during pre/post_reset and suspend/resume
and while we hold any locks these methods take. This is not simple and
people need to be reminded.

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Montag, 4. Mai 2009 09:02:38 schrieb David Brownell:
> On Sunday 03 May 2009, Oliver Neukum wrote:
> > No. To make it plain. To me any use of memdup_user() in USB code
> > is a bad idea. I don't want to have to think about a new primitive.
>
> Unless it's incorrect to use that, I have to say that it
> makes more sense to use that utility than recreate it by
> open-coding...

I want people to be forced to think about memory allocations.
We had endless trouble during 2.4 with storage deadlocking.
We simply need full control of this.

I prefer the explicite way for the same reason I prefer

if (rv < 0)
	goto err_out;

over

if (rv < 0)
	return rv;

In the former version you need to think about what you need to do
to cleanup.

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[Andrew Morton]

On Mon, 4 May 2009 16:01:51 +0200 Oliver Neukum <oliver@neukum.org> wrote:

> Am Montag, 4. Mai 2009 09:02:38 schrieb David Brownell:
> > On Sunday 03 May 2009, Oliver Neukum wrote:
> > > No. To make it plain. To me any use of memdup_user() in USB code
> > > is a bad idea. I don't want to have to think about a new primitive.
> >
> > Unless it's incorrect to use that, I have to say that it
> > makes more sense to use that utility than recreate it by
> > open-coding...
> 
> I want people to be forced to think about memory allocations.
> We had endless trouble during 2.4 with storage deadlocking.
> We simply need full control of this.
> 

thou-shalt-use-GFP_NOFS is a very common pattern in many filesystems. 
And thou-shalt-use-GFP_NOIO is a very common pattern in block drivers.

I wonder how hard it would be to add runtime debugging checks?  If
there are clearly identified transition points where (say) GFP_NOIO
becomes required and unrequired then we could do something along the
lines of

	current->disallowed_gfp_flags |= __GFP_IO;
	....
	current->disallowed_gfp_flags &= ~__GFP_IO;

and check (gfp_flags & current->disallowed_gfp_flags) in the various
memory-allocation functions, and perhaps in the uaccess functions.

Or possibly teach lockdep about it, although that seems inappropriate.

Anyway.  A little project for someone, perhaps.

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Dienstag, 5. Mai 2009 08:11:57 schrieb Andrew Morton:
> On Mon, 4 May 2009 16:01:51 +0200 Oliver Neukum <oliver@neukum.org> wrote:

> > I want people to be forced to think about memory allocations.
> > We had endless trouble during 2.4 with storage deadlocking.
> > We simply need full control of this.
>
> thou-shalt-use-GFP_NOFS is a very common pattern in many filesystems.
> And thou-shalt-use-GFP_NOIO is a very common pattern in block drivers.

USB drivers are interface level yet some functions, reset and power
management, are on a device level. As it is unpredictable whether
a driver will share a device with a storage driver, all USB drivers as far as
these functions are concerned must be considered block device drivers.
That's the reason GFP_NOIO is so prevalent in USB.

> I wonder how hard it would be to add runtime debugging checks?  If

I'd prefer compile time checks. Ideally we'd annotate a function with an
attribute making the compiler barf if copy_to/from_user or an inappropriate
kmalloc is used. It can't be perfect due to function pointers, but it would
be a good start.

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[Andrew Morton]

On Tue, 5 May 2009 12:44:01 +0200 Oliver Neukum <oliver@neukum.org> wrote:

> Am Dienstag, 5. Mai 2009 08:11:57 schrieb Andrew Morton:
> > On Mon, 4 May 2009 16:01:51 +0200 Oliver Neukum <oliver@neukum.org> wrote:
> 
> > > I want people to be forced to think about memory allocations.
> > > We had endless trouble during 2.4 with storage deadlocking.
> > > We simply need full control of this.
> >
> > thou-shalt-use-GFP_NOFS is a very common pattern in many filesystems.
> > And thou-shalt-use-GFP_NOIO is a very common pattern in block drivers.
> 
> USB drivers are interface level yet some functions, reset and power
> management, are on a device level. As it is unpredictable whether
> a driver will share a device with a storage driver, all USB drivers as far as
> these functions are concerned must be considered block device drivers.
> That's the reason GFP_NOIO is so prevalent in USB.

There must be some particular action which flips the thread of control
from one state to the other.  eg, taking of a lock.

> > I wonder how hard it would be to add runtime debugging checks?  If
> 
> I'd prefer compile time checks. Ideally we'd annotate a function with an
> attribute making the compiler barf if copy_to/from_user or an inappropriate
> kmalloc is used. It can't be perfect due to function pointers, but it would
> be a good start.

I don't think that would have enough coverage - bugs in this area tend
to come from calling some function which looks innocent, but which
calls some function which calls some function which calls some function
which uses GFP_KERNEL.

And then there's stuff like "usb takes a mutex which is also taken by
some other thread which does a GFP_KERNEL allocation while holding that
mutex".

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Dienstag, 5. Mai 2009 19:22:53 schrieb Andrew Morton:
> On Tue, 5 May 2009 12:44:01 +0200 Oliver Neukum <oliver@neukum.org> wrote:

> > USB drivers are interface level yet some functions, reset and power
> > management, are on a device level. As it is unpredictable whether
> > a driver will share a device with a storage driver, all USB drivers as
> > far as these functions are concerned must be considered block device
> > drivers. That's the reason GFP_NOIO is so prevalent in USB.
>
> There must be some particular action which flips the thread of control
> from one state to the other.  eg, taking of a lock.

Basically assigning an interface to the storage or ub driver.

> > > I wonder how hard it would be to add runtime debugging checks?  If
> >
> > I'd prefer compile time checks. Ideally we'd annotate a function with an
> > attribute making the compiler barf if copy_to/from_user or an
> > inappropriate kmalloc is used. It can't be perfect due to function
> > pointers, but it would be a good start.
>
> I don't think that would have enough coverage - bugs in this area tend
> to come from calling some function which looks innocent, but which
> calls some function which calls some function which calls some function
> which uses GFP_KERNEL.
>
> And then there's stuff like "usb takes a mutex which is also taken by
> some other thread which does a GFP_KERNEL allocation while holding that
> mutex".

Yes, but to catch that you'd have to teach lockdep about those functions
whose locks are dangerous to share with respect to memory allocation.
Is there another way to do that besides labelling dangerous methods?

	Regards
		Oliver

Re: [PATCH] usb: use memdup_user()

[Andrew Morton]

On Wed, 6 May 2009 15:34:52 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> Am Dienstag, 5. Mai 2009 19:22:53 schrieb Andrew Morton:
> > On Tue, 5 May 2009 12:44:01 +0200 Oliver Neukum <oliver@neukum.org> wrote:
> 
> > > USB drivers are interface level yet some functions, reset and power
> > > management, are on a device level. As it is unpredictable whether
> > > a driver will share a device with a storage driver, all USB drivers as
> > > far as these functions are concerned must be considered block device
> > > drivers. That's the reason GFP_NOIO is so prevalent in USB.
> >
> > There must be some particular action which flips the thread of control
> > from one state to the other.  eg, taking of a lock.
> 
> Basically assigning an interface to the storage or ub driver.

That's hardly enough information for anyone to understand what you
mean :(

Oh well, doesn't matter.

> > > > I wonder how hard it would be to add runtime debugging checks?  If
> > >
> > > I'd prefer compile time checks. Ideally we'd annotate a function with an
> > > attribute making the compiler barf if copy_to/from_user or an
> > > inappropriate kmalloc is used. It can't be perfect due to function
> > > pointers, but it would be a good start.
> >
> > I don't think that would have enough coverage - bugs in this area tend
> > to come from calling some function which looks innocent, but which
> > calls some function which calls some function which calls some function
> > which uses GFP_KERNEL.
> >
> > And then there's stuff like "usb takes a mutex which is also taken by
> > some other thread which does a GFP_KERNEL allocation while holding that
> > mutex".
> 
> Yes, but to catch that you'd have to teach lockdep about those functions
> whose locks are dangerous to share with respect to memory allocation.
> Is there another way to do that besides labelling dangerous methods?

Adding lockdep annotation to the locks, I guess.  Probably a new kind of
annotation.

Re: [PATCH] usb: use memdup_user()

[Oliver Neukum]

Am Mittwoch, 6. Mai 2009 20:31:17 schrieb Andrew Morton:
> > > There must be some particular action which flips the thread of control
> > > from one state to the other.  eg, taking of a lock.
> >
> > Basically assigning an interface to the storage or ub driver.
>
> That's hardly enough information for anyone to understand what you
> mean :(

But isn't that always the same? If you take a lock that a method in the block
IO takes, you must use GFP_NOIO. What exactly do you want me to explain
better?

	Regards
		Oliver