On Thu, 7 May 2020 10:58:06 +0200
Oliver Neukum <oneukum@xxxxxxxx> wrote:
> syzkaller reported an URB that should have been killed to be active.
> We do not understand it, but this should fix the issue if it is real.
> @@ -1375,9 +1376,11 @@ static void usblp_disconnect(struct usb_interface *intf)
>
> usblp_unlink_urbs(usblp);
> mutex_unlock(&usblp->mut);
> + usb_poison_anchored_urbs(&usblp->urbs);
>
> if (!usblp->used)
> usblp_cleanup(usblp);
> mutex_unlock(&usblp_mutex);
> }
Sorry, it took me this long to think about it. I am against the duplication
of effort between usblp_unlink_urbs, which is exactly usb_kill_anchored_urbs,
and usb_poison_anchored_urbs. If you think that poisoning may help against
what the bot identified, we may try this instead:
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 0d8e3f3804a3..eae5eaf5d4f1 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -1373,7 +1373,7 @@ static void usblp_disconnect(struct usb_interface *intf)
wake_up(&usblp->rwait);
usb_set_intfdata(intf, NULL);
- usblp_unlink_urbs(usblp);
+ usb_poison_anchored_urbs(&usblp->urbs);
mutex_unlock(&usblp->mut);
if (!usblp->used)
I'm still concerned that we didn't identify the scenario tht led to bot's
findings.
The usblp->present was supposed to play a role of the poison pill, at the
driver level. The difference with poisoning the anchor is that ->present
is protected by the most outlying mutex, and therefore cannot be meaninfully
checked in URB callbacks. But the anchor's poison flag is protected by a
spinlock, so callbacks check it. But what does it matter for us? This driver
does not re-submit URBs from callbacks.
So, I'm suspicious of attempts to hit at the problem in the dark and hope
for a miracle.
-- Pete
