On Tue, Apr 13, 2021 at 12:13:11PM -0400, Alan Stern wrote: > On Tue, Apr 13, 2021 at 10:12:05AM +0200, Dmitry Vyukov wrote: > > On Tue, Apr 13, 2021 at 10:08 AM syzbot > > <syzbot+eb4674092e6cc8d9e0bd@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > Hello, > > > > > > syzbot found the following issue on: > > > > > > HEAD commit: 0f4498ce Merge tag 'for-5.12/dm-fixes-2' of git://git.kern.. > > > git tree: upstream > > > console output: https://syzkaller.appspot.com/x/log.txt?x=124adbf6d00000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=daeff30c2474a60f > > > dashboard link: https://syzkaller.appspot.com/bug?extid=eb4674092e6cc8d9e0bd > > > userspace arch: i386 > > > > > > Unfortunately, I don't have any reproducer for this issue yet. > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > > Reported-by: syzbot+eb4674092e6cc8d9e0bd@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > I suspect that the raw gadget_unbind() can be called while the timer > > is still active. gadget_unbind() sets gadget data to NULL. > > But I am not sure which unbind call this is: > > usb_gadget_remove_driver() or right in udc_bind_to_driver() due to a > > start error. > > This certainly looks like a race between gadget_unbind and gadget_setup > in raw_gadget. > > In theory, this race shouldn't matter. The gadget core is supposed to > guarantee that there won't be any more callbacks to the gadget driver > once the driver's unbind routine is called. That guarantee is enforced > in usb_gadget_remove_driver as follows: > > usb_gadget_disconnect(udc->gadget); > if (udc->gadget->irq) > synchronize_irq(udc->gadget->irq); > udc->driver->unbind(udc->gadget); > usb_gadget_udc_stop(udc); > > usb_gadget_disconnect turns off the pullup resistor, telling the host > that the gadget is no longer connected and preventing the transmission > of any more USB packets. Any packets that have already been received > are sure to processed by the UDC driver's interrupt handler by the time > synchronize_irq returns. > > But this doesn't work with dummy_hcd, because dummy_hcd doesn't use > interrupts; it uses a timer instead. It does have code to emulate the > effect of synchronize_irq, but that code doesn't get invoked at the > right time -- it currently runs in usb_gadget_udc_stop, after the unbind > callback instead of before. Indeed, there's no way for > usb_gadget_remove_driver to invoke this code before the unbind > callback,. > > I thought the synchronize_irq emulation problem had been completely > solved, but evidently it hasn't. It looks like the best solution is to > add a call of the synchronize_irq emulation code in dummy_pullup. > > Maybe we can test this reasoning by putting a delay just before the call > to dum->driver->setup. That runs in the timer handler, so it's not a > good place to delay, but it may be okay just for testing purposes. > > Hopefully this patch will make the race a lot more likely to occur. Is Hi Alan, Indeed, I was able to reproduce this bug easily on my machine with your delay patch applied and using this syzkaller program: syz_usb_connect$cdc_ncm(0x1, 0x6e, &(0x7f0000000040)={{0x12, 0x1, 0x0, 0x2, 0x0, 0x0, 0x8, 0x525, 0xa4a1, 0x40, 0x1, 0x2, 0x3, 0x1, [{{0x9, 0x2, 0x5c, 0x2, 0x1, 0x0, 0x0, 0x0, {{0x9, 0x4, 0x0, 0x0, 0x1, 0x2, 0xd, 0x0, 0x0, {{0x5}, {0x5}, {0xd}, {0x6}}, {{0x9, 0x5, 0x81, 0x3, 0x200}}}}}}]}}, &(0x7f0000000480)={0x0, 0x0, 0x0, 0x0, 0x3, [{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}]}) I also tested doing the synchronize_irq emulation in dummy_pullup and it fixed the issue. The patch is below. Thanks! - Anirudh. diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index ce24d4f28f2a..931d4612d859 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -903,6 +903,12 @@ static int dummy_pullup(struct usb_gadget *_gadget, int value) spin_lock_irqsave(&dum->lock, flags); dum->pullup = (value != 0); set_link_state(dum_hcd); + /* emulate synchronize_irq(): wait for callbacks to finish */ + while (dum->callback_usage > 0) { + spin_unlock_irqrestore(&dum->lock, flags); + usleep_range(1000, 2000); + spin_lock_irqsave(&dum->lock, flags); + } spin_unlock_irqrestore(&dum->lock, flags); usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd)); @@ -1005,13 +1011,6 @@ static int dummy_udc_stop(struct usb_gadget *g) dum->ints_enabled = 0; stop_activity(dum); - /* emulate synchronize_irq(): wait for callbacks to finish */ - while (dum->callback_usage > 0) { - spin_unlock_irq(&dum->lock); - usleep_range(1000, 2000); - spin_lock_irq(&dum->lock); - } - dum->driver = NULL; spin_unlock_irq(&dum->lock); @@ -1900,6 +1899,7 @@ static void dummy_timer(struct timer_list *t) if (value > 0) { ++dum->callback_usage; spin_unlock(&dum->lock); + mdelay(5); value = dum->driver->setup(&dum->gadget, &setup); spin_lock(&dum->lock);
