| Message ID | 20200507085806.5793-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 296a193b06120aa6ae7cf5c0d7b5e5b55968026e |
| Headers | show |
| Series | usblp: poison URBs upon disconnect | expand |
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
Am Freitag, den 15.05.2020, 15:31 -0500 schrieb Pete Zaitcev: > > and usb_poison_anchored_urbs. If you think that poisoning may help against > what the bot identified, we may try this instead: Sure. Would you send in a patch? > I'm still concerned that we didn't identify the scenario tht led to bot's > findings. So am I. Yet as far as I can tell the code of usblp is correct. Even worse, it is casting doubt on our testing framework. > 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. It also makes resubmission impossible. In fact, considering that, we might better poison before we go for usblp->present. > So, I'm suspicious of attempts to hit at the problem in the dark and hope > for a miracle. Right. The only thing worse would be doing nothing. At the risk of repeating myself, usblp looks correct to me. Regards Oliver
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c index 0d8e3f3804a3..084c48c5848f 100644 --- a/drivers/usb/class/usblp.c +++ b/drivers/usb/class/usblp.c @@ -468,7 +468,8 @@ static int usblp_release(struct inode *inode, struct file *file) usb_autopm_put_interface(usblp->intf); if (!usblp->present) /* finish cleanup from disconnect */ - usblp_cleanup(usblp); + usblp_cleanup(usblp); /* any URBs must be dead */ + mutex_unlock(&usblp_mutex); return 0; } @@ -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); }
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. Signed-off-by: Oliver Neukum <oneukum@suse.com> Reported-by: syzbot+be5b5f86a162a6c281e6@syzkaller.appspotmail.com --- drivers/usb/class/usblp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)