| Message ID | 20180901081210.16655-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt() | expand |
On 2018-09-01 16:12:10 [+0800], Jia-Ju Bai wrote: > wdm_in_callback() is a completion handler function for the USB driver. > So it should not sleep. But it calls service_outstanding_interrupt(), > which calls usb_submit_urb() with GFP_KERNEL. At which point does wdm_in_callback() invoke service_outstanding_interrupt()? I don't see it. I see one invocation from wdm_read() and another from service_interrupt_work(). Also, if that would be the case, then spin_unlock_irq() in an USB completion handler (which might run in IRQ context with interrupts disabled) would be wrong. > To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. > > This bug is found by my static analysis tool DSAC. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > v2: > * Add more description. > --- > drivers/usb/class/cdc-wdm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > index a0d284ef3f40..632a2bfabc08 100644 > --- a/drivers/usb/class/cdc-wdm.c > +++ b/drivers/usb/class/cdc-wdm.c > @@ -458,7 +458,7 @@ static int service_outstanding_interrupt(struct wdm_device *desc) > > set_bit(WDM_RESPONDING, &desc->flags); > spin_unlock_irq(&desc->iuspin); > - rv = usb_submit_urb(desc->response, GFP_KERNEL); > + rv = usb_submit_urb(desc->response, GFP_ATOMIC); > spin_lock_irq(&desc->iuspin); > if (rv) { > dev_err(&desc->intf->dev, Sebastian
On 2018/9/11 15:49, Sebastian Andrzej Siewior wrote: > On 2018-09-01 16:12:10 [+0800], Jia-Ju Bai wrote: >> wdm_in_callback() is a completion handler function for the USB driver. >> So it should not sleep. But it calls service_outstanding_interrupt(), >> which calls usb_submit_urb() with GFP_KERNEL. > At which point does wdm_in_callback() invoke > service_outstanding_interrupt()? I don't see it. I see one invocation > from wdm_read() and another from service_interrupt_work(). > > Also, if that would be the case, then spin_unlock_irq() in an USB > completion handler (which might run in IRQ context with interrupts > disabled) would be wrong. Yes, you are right. I checked an old kernel version Linux-4.16 and got this report. The current code looks much different from the code that I checked. Sorry for my false report, and thanks for your correction. Best wishes, Jia-Ju Bai
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index a0d284ef3f40..632a2bfabc08 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -458,7 +458,7 @@ static int service_outstanding_interrupt(struct wdm_device *desc) set_bit(WDM_RESPONDING, &desc->flags); spin_unlock_irq(&desc->iuspin); - rv = usb_submit_urb(desc->response, GFP_KERNEL); + rv = usb_submit_urb(desc->response, GFP_ATOMIC); spin_lock_irq(&desc->iuspin); if (rv) { dev_err(&desc->intf->dev,
wdm_in_callback() is a completion handler function for the USB driver. So it should not sleep. But it calls service_outstanding_interrupt(), which calls usb_submit_urb() with GFP_KERNEL. To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. This bug is found by my static analysis tool DSAC. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- v2: * Add more description. --- drivers/usb/class/cdc-wdm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)