[PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alexey Khoroshilov]

As it was described by Oliver Neukum in commit acbe2fe
"USB: Don't use GFP_KERNEL while we cannot reset a storage device":

  Memory allocations with GFP_KERNEL can cause IO to a storage device
  which can fail resulting in a need to reset the device. Therefore
  GFP_KERNEL cannot be safely used between usb_lock_device()
  and usb_unlock_device(). Replace by GFP_NOIO.

The patch fixes the same issue in usb/core/devio.c.
All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/usb/core/devio.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 8823e98..4be27e3 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -275,10 +275,10 @@ static struct async *alloc_async(unsigned int numisoframes)
 {
 	struct async *as;
 
-	as = kzalloc(sizeof(struct async), GFP_KERNEL);
+	as = kzalloc(sizeof(struct async), GFP_NOIO);
 	if (!as)
 		return NULL;
-	as->urb = usb_alloc_urb(numisoframes, GFP_KERNEL);
+	as->urb = usb_alloc_urb(numisoframes, GFP_NOIO);
 	if (!as->urb) {
 		kfree(as);
 		return NULL;
@@ -887,7 +887,7 @@ static int proc_control(struct dev_state *ps, void __user *arg)
 			sizeof(struct usb_ctrlrequest));
 	if (ret)
 		return ret;
-	tbuf = (unsigned char *)__get_free_page(GFP_KERNEL);
+	tbuf = (unsigned char *)__get_free_page(GFP_NOIO);
 	if (!tbuf) {
 		ret = -ENOMEM;
 		goto done;
@@ -983,7 +983,7 @@ static int proc_bulk(struct dev_state *ps, void __user *arg)
 	ret = usbfs_increase_memory_usage(len1 + sizeof(struct urb));
 	if (ret)
 		return ret;
-	if (!(tbuf = kmalloc(len1, GFP_KERNEL))) {
+	if (!(tbuf = kmalloc(len1, GFP_NOIO))) {
 		ret = -ENOMEM;
 		goto done;
 	}
@@ -1211,7 +1211,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 		/* min 8 byte setup packet */
 		if (uurb->buffer_length < 8)
 			return -EINVAL;
-		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
+		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
 		if (!dr)
 			return -ENOMEM;
 		if (copy_from_user(dr, uurb->buffer, 8)) {
@@ -1278,7 +1278,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 			return -EINVAL;
 		isofrmlen = sizeof(struct usbdevfs_iso_packet_desc) *
 				   uurb->number_of_packets;
-		if (!(isopkt = kmalloc(isofrmlen, GFP_KERNEL)))
+		if (!(isopkt = kmalloc(isofrmlen, GFP_NOIO)))
 			return -ENOMEM;
 		if (copy_from_user(isopkt, iso_frame_desc, isofrmlen)) {
 			ret = -EFAULT;
@@ -1326,7 +1326,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 
 	if (num_sgs) {
 		as->urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist),
-				      GFP_KERNEL);
+				      GFP_NOIO);
 		if (!as->urb->sg) {
 			ret = -ENOMEM;
 			goto error;
@@ -1337,7 +1337,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 		totlen = uurb->buffer_length;
 		for (i = 0; i < as->urb->num_sgs; i++) {
 			u = (totlen > USB_SG_SIZE) ? USB_SG_SIZE : totlen;
-			buf = kmalloc(u, GFP_KERNEL);
+			buf = kmalloc(u, GFP_NOIO);
 			if (!buf) {
 				ret = -ENOMEM;
 				goto error;
@@ -1355,7 +1355,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 		}
 	} else if (uurb->buffer_length > 0) {
 		as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
-				GFP_KERNEL);
+				GFP_NOIO);
 		if (!as->urb->transfer_buffer) {
 			ret = -ENOMEM;
 			goto error;
@@ -1467,7 +1467,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 			ret = usb_submit_urb(as->urb, GFP_ATOMIC);
 		spin_unlock_irq(&ps->lock);
 	} else {
-		ret = usb_submit_urb(as->urb, GFP_KERNEL);
+		ret = usb_submit_urb(as->urb, GFP_NOIO);
 	}
 
 	if (ret) {
@@ -1798,7 +1798,7 @@ static int proc_ioctl(struct dev_state *ps, struct usbdevfs_ioctl *ctl)
 
 	/* alloc buffer */
 	if ((size = _IOC_SIZE(ctl->ioctl_code)) > 0) {
-		if ((buf = kmalloc(size, GFP_KERNEL)) == NULL)
+		if ((buf = kmalloc(size, GFP_NOIO)) == NULL)
 			return -ENOMEM;
 		if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
 			if (copy_from_user(buf, ctl->data, size)) {
-- 
1.7.9.5

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alan Stern]

On Sat, 9 Mar 2013, Alexey Khoroshilov wrote:

> As it was described by Oliver Neukum in commit acbe2fe
> "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
> 
>   Memory allocations with GFP_KERNEL can cause IO to a storage device
>   which can fail resulting in a need to reset the device. Therefore
>   GFP_KERNEL cannot be safely used between usb_lock_device()
>   and usb_unlock_device(). Replace by GFP_NOIO.
> 
> The patch fixes the same issue in usb/core/devio.c.
> All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().
> 
> Found by Linux Driver Verification project (linuxtesting.org).

I don't know if this is a good idea.  People can and do submit 
transfers requiring a lot of buffer space.  Switching to GFP_NOIO 
will make those allocations a lot more likely to fail.

Oliver, what do you think?

Alan Stern

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Oliver Neukum]

On Friday 08 March 2013 12:55:08 Alan Stern wrote:
> On Sat, 9 Mar 2013, Alexey Khoroshilov wrote:
> 
> > As it was described by Oliver Neukum in commit acbe2fe
> > "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
> > 
> >   Memory allocations with GFP_KERNEL can cause IO to a storage device
> >   which can fail resulting in a need to reset the device. Therefore
> >   GFP_KERNEL cannot be safely used between usb_lock_device()
> >   and usb_unlock_device(). Replace by GFP_NOIO.
> > 
> > The patch fixes the same issue in usb/core/devio.c.
> > All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().
> > 
> > Found by Linux Driver Verification project (linuxtesting.org).
> 
> I don't know if this is a good idea.  People can and do submit 
> transfers requiring a lot of buffer space.  Switching to GFP_NOIO 
> will make those allocations a lot more likely to fail.
> 
> Oliver, what do you think?

Ideally we'd split memory allocation and use, by it fixes a bug.
Better allocation failure than deadlock.

	Regards
		Oliver

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Greg Kroah-Hartman]

On Sat, Mar 09, 2013 at 12:50:47AM +0800, Alexey Khoroshilov wrote:
> As it was described by Oliver Neukum in commit acbe2fe
> "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
> 
>   Memory allocations with GFP_KERNEL can cause IO to a storage device
>   which can fail resulting in a need to reset the device. Therefore
>   GFP_KERNEL cannot be safely used between usb_lock_device()
>   and usb_unlock_device(). Replace by GFP_NOIO.
> 
> The patch fixes the same issue in usb/core/devio.c.

But that's for userspace drivers, why does it matter here for them?

> All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().

I really don't want to have usbfs using GFP_NOIO as it is known for
allocating a lot of memory already.  Given that no one has ever reported
a problem like this, I don't think it deserves to change, sorry.

greg k-h

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alan Stern]

On Fri, 8 Mar 2013, Oliver Neukum wrote:

> On Friday 08 March 2013 12:55:08 Alan Stern wrote:
> > On Sat, 9 Mar 2013, Alexey Khoroshilov wrote:
> > 
> > > As it was described by Oliver Neukum in commit acbe2fe
> > > "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
> > > 
> > >   Memory allocations with GFP_KERNEL can cause IO to a storage device
> > >   which can fail resulting in a need to reset the device. Therefore
> > >   GFP_KERNEL cannot be safely used between usb_lock_device()
> > >   and usb_unlock_device(). Replace by GFP_NOIO.
> > > 
> > > The patch fixes the same issue in usb/core/devio.c.
> > > All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().
> > > 
> > > Found by Linux Driver Verification project (linuxtesting.org).
> > 
> > I don't know if this is a good idea.  People can and do submit 
> > transfers requiring a lot of buffer space.  Switching to GFP_NOIO 
> > will make those allocations a lot more likely to fail.
> > 
> > Oliver, what do you think?
> 
> Ideally we'd split memory allocation and use, by it fixes a bug.
> Better allocation failure than deadlock.

In fact we wouldn't deadlock.  This is because 
usb_lock_device_for_reset() gives up if it can't obtain the device lock 
after one second of trying.  We'd just end up with a failure to reset, 
leading to an I/O failure.

Probably the mass-storage device would be taken off-line...  but there
wouldn't be a deadlock.  Under the circumstances, I'd say that the 
consequences of merging this patch would be worse than the consequences 
of keeping things as they are now.

Alan Stern

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Ming Lei]

On Sat, Mar 9, 2013 at 12:50 AM, Alexey Khoroshilov
<khoroshilov@ispras.ru> wrote:
> As it was described by Oliver Neukum in commit acbe2fe
> "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
>
>   Memory allocations with GFP_KERNEL can cause IO to a storage device
>   which can fail resulting in a need to reset the device. Therefore
>   GFP_KERNEL cannot be safely used between usb_lock_device()
>   and usb_unlock_device(). Replace by GFP_NOIO.
>
> The patch fixes the same issue in usb/core/devio.c.
> All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().

I am wondering why the device lock is needed for usbdev_do_ioctl()? Looks
device lock isn't required for USB transfer of kernel driver.


Thanks,
-- 
Ming Lei

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alan Stern]

On Mon, 11 Mar 2013, Ming Lei wrote:

> On Sat, Mar 9, 2013 at 12:50 AM, Alexey Khoroshilov
> <khoroshilov@ispras.ru> wrote:
> > As it was described by Oliver Neukum in commit acbe2fe
> > "USB: Don't use GFP_KERNEL while we cannot reset a storage device":
> >
> >   Memory allocations with GFP_KERNEL can cause IO to a storage device
> >   which can fail resulting in a need to reset the device. Therefore
> >   GFP_KERNEL cannot be safely used between usb_lock_device()
> >   and usb_unlock_device(). Replace by GFP_NOIO.
> >
> > The patch fixes the same issue in usb/core/devio.c.
> > All the allocations fixed are under usb_lock_device() from usbdev_do_ioctl().
> 
> I am wondering why the device lock is needed for usbdev_do_ioctl()? Looks
> device lock isn't required for USB transfer of kernel driver.

Of course you have to lock the device before changing its driver.  What 
would happen if two different threads tried to change a device's driver 
at the same time?

usbdev_do_ioctl() needs to acquire the device lock in order to prevent 
races with driver_disconnect() and usbdev_remove().

Alan Stern

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Ming Lei]

On Mon, Mar 11, 2013 at 11:32 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Of course you have to lock the device before changing its driver.  What
> would happen if two different threads tried to change a device's driver
> at the same time?

Yes, claim/release interface need device lock, but the patch doesn't
touch claim/release command handling.

>
> usbdev_do_ioctl() needs to acquire the device lock in order to prevent
> races with driver_disconnect() and usbdev_remove().

Looks the patch basically converts the allocation inside URB submit path,
and actually I mean why we need to hold device lock in submitting
URB path?  Device lock isn't required before submitting URBs
in kernel driver.


Thanks,
-- 
Ming Lei

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alan Stern]

On Mon, 11 Mar 2013, Ming Lei wrote:

> On Mon, Mar 11, 2013 at 11:32 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > Of course you have to lock the device before changing its driver.  What
> > would happen if two different threads tried to change a device's driver
> > at the same time?
> 
> Yes, claim/release interface need device lock, but the patch doesn't
> touch claim/release command handling.

Then why did you ask?  You wrote: "Looks device lock isn't required for
USB transfer of kernel driver."


> > usbdev_do_ioctl() needs to acquire the device lock in order to prevent
> > races with driver_disconnect() and usbdev_remove().
> 
> Looks the patch basically converts the allocation inside URB submit path,
> and actually I mean why we need to hold device lock in submitting
> URB path?  Device lock isn't required before submitting URBs
> in kernel driver.

In general it isn't, no.  But usbfs uses the lock to prevent races with 
driver_disconnect() -- it is invalid to submit URBs after the 
disconnect routine has returned.

Alan Stern

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Ming Lei]

On Tue, Mar 12, 2013 at 12:08 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 11 Mar 2013, Ming Lei wrote:
>
>> On Mon, Mar 11, 2013 at 11:32 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >
>> > Of course you have to lock the device before changing its driver.  What
>> > would happen if two different threads tried to change a device's driver
>> > at the same time?
>>
>> Yes, claim/release interface need device lock, but the patch doesn't
>> touch claim/release command handling.
>
> Then why did you ask?  You wrote: "Looks device lock isn't required for
> USB transfer of kernel driver."

Maybe I didn't expressed clearly, sorry. Here the USB transfer means
URBs submitting.

>
>
>> > usbdev_do_ioctl() needs to acquire the device lock in order to prevent
>> > races with driver_disconnect() and usbdev_remove().
>>
>> Looks the patch basically converts the allocation inside URB submit path,
>> and actually I mean why we need to hold device lock in submitting
>> URB path?  Device lock isn't required before submitting URBs
>> in kernel driver.
>
> In general it isn't, no.  But usbfs uses the lock to prevent races with
> driver_disconnect() -- it is invalid to submit URBs after the
> disconnect routine has returned.

If so, we may introduce another lock to avoid the race.

So I think we may figure out to decrease the device lock
scope in devio.c, and most of ioctl commands might not require it
at all, then the problem addressed by the patch can be fixed too.


Thanks,
-- 
Ming Lei

Re: [PATCH] usb/core/devio.c: Don't use GFP_KERNEL while we cannot reset a storage device

[Alan Stern]

On Tue, 12 Mar 2013, Ming Lei wrote:

> > In general it isn't, no.  But usbfs uses the lock to prevent races with
> > driver_disconnect() -- it is invalid to submit URBs after the
> > disconnect routine has returned.
> 
> If so, we may introduce another lock to avoid the race.
> 
> So I think we may figure out to decrease the device lock
> scope in devio.c, and most of ioctl commands might not require it
> at all, then the problem addressed by the patch can be fixed too.

That might work.  A mutex could be added to the dev_state structure.  
The mutex would be locked whenever an URB was being submitted and
during driver_disconnect, and perhaps a few other places too, but not
when memory was being allocated.  The device itself would remain 
unlocked most of the time.

Alan Stern