On Wed, 22 Apr 2020 11:23:23 +0800
Hillf Danton <hdanton@xxxxxxxx> wrote:
> Do cleanup for lp after submitted urb completes.
>
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -1376,8 +1376,10 @@ static void usblp_disconnect(struct usb_
> usblp_unlink_urbs(usblp);
> mutex_unlock(&usblp->mut);
>
> - if (!usblp->used)
> + if (!usblp->used) {
> + wait_event(usblp->rwait, usblp->rcomplete != 0);
> usblp_cleanup(usblp);
> + }
> mutex_unlock(&usblp_mutex);
> }
I do not agree with this kind of workaround. The model we're following
is for usb_kill_urb() to cancel the transfer. The usblp invokes it
through usb_kill_anchored_urbs() and usblp_unlink_urbs(), as seen
above. There can be no timer hitting anything once it returns.
At this point I suspect the fake HCD that the test harness invokes
fails to termine the transfer properly and then a timer hits.
Here's the bot's evidence and how I read it:
> Tue, 21 Apr 2020 08:35:18 -0700
> > Reported-by: syzbot+be5b5f86a162a6c281e6@xxxxxxxxxxxxxxxxxxxxxxxxx
This is where the problem is tripped, notice that it comes
because gadget runs a timer:
> > kasan_report+0xe/0x20 mm/kasan/common.c:641
> > __lock_acquire+0x31af/0x3b60 kernel/locking/lockdep.c:3827
> > lock_acquire+0x130/0x340 kernel/locking/lockdep.c:4484
> > __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> > _raw_spin_lock_irqsave+0x32/0x50 kernel/locking/spinlock.c:159
> > usblp_bulk_read+0x211/0x270 drivers/usb/class/usblp.c:303
> > __usb_hcd_giveback_urb+0x1f2/0x470 drivers/usb/core/hcd.c:1648
> > usb_hcd_giveback_urb+0x368/0x420 drivers/usb/core/hcd.c:1713
> > dummy_timer+0x1258/0x32ae drivers/usb/gadget/udc/dummy_hcd.c:1966
> > call_timer_fn+0x195/0x6f0 kernel/time/timer.c:1404
> > expire_timers kernel/time/timer.c:1449 [inline]
> > __run_timers kernel/time/timer.c:1773 [inline]
> > __run_timers kernel/time/timer.c:1740 [inline]
At this point the whole struct usblp is freed, including the
spinlock which we're trying to lock.
> > Allocated by task 3361:
> > save_stack+0x1b/0x80 mm/kasan/common.c:72
> > set_track mm/kasan/common.c:80 [inline]
> > __kasan_kmalloc mm/kasan/common.c:515 [inline]
> > __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:488
> > kmalloc include/linux/slab.h:555 [inline]
> > kzalloc include/linux/slab.h:669 [inline]
> > usblp_probe+0xed/0x1200 drivers/usb/class/usblp.c:1104
> > usb_probe_interface+0x310/0x800 drivers/usb/core/driver.c:374
> > really_probe+0x290/0xac0 drivers/base/dd.c:551
> > driver_probe_device+0x223/0x350 drivers/base/dd.c:724
1104 is kzalloc for struct usblp.
> > Freed by task 12266:
> > save_stack+0x1b/0x80 mm/kasan/common.c:72
> > set_track mm/kasan/common.c:80 [inline]
> > kasan_set_free_info mm/kasan/common.c:337 [inline]
> > __kasan_slab_free+0x117/0x160 mm/kasan/common.c:476
> > slab_free_hook mm/slub.c:1444 [inline]
> > slab_free_freelist_hook mm/slub.c:1477 [inline]
> > slab_free mm/slub.c:3034 [inline]
> > kfree+0xd5/0x300 mm/slub.c:3995
> > usblp_disconnect.cold+0x24/0x29 drivers/usb/class/usblp.c:1380
> > usb_unbind_interface+0x1bd/0x8a0 drivers/usb/core/driver.c:436
> > __device_release_driver drivers/base/dd.c:1137 [inline]
> > device_release_driver_internal+0x42f/0x500 drivers/base/dd.c:1168
> > bus_remove_device+0x2eb/0x5a0 drivers/base/bus.c:533
1380 is an inlined call to usblp_cleanup, which is just
a bunch of kfree.
The bug report is still a bug report, but I'm pretty sure the
culprit is the emulated HCD and/or the gadget layer. Unfortunately,
I'm not up to speed in that subsystem. Maybe Alan can look at it?
-- Pete
