Re: [PATCH] usblp: poison URBs upon disconnect

[Pete Zaitcev <zaitcev@redhat.com>]

On Thu,  7 May 2020 10:58:06 +0200
Oliver Neukum <oneukum@suse.com> 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