Re: [PATCH 0/6] usbip fixes to crashes found by syzbot

[Shuah Khan]

On 3/17/21 9:06 AM, Shuah Khan wrote:
> On 3/17/21 12:21 AM, Tetsuo Handa wrote:
> > Shuah, this driver is getting more and more cryptic and buggy.
> > Please explain the strategy for serialization before you write patches.
> >
> > > - Fix attach_store() to check usbip_event_happened() before
> > >    waking up threads.
> >
> > No, this helps nothing.
> >
> > > diff --git a/drivers/usb/usbip/vhci_sysfs.c 
> > > b/drivers/usb/usbip/vhci_sysfs.c
> > > index c4b4256e5dad3..f0a770adebd97 100644
> > > --- a/drivers/usb/usbip/vhci_sysfs.c
> > > +++ b/drivers/usb/usbip/vhci_sysfs.c
> > > @@ -418,6 +418,15 @@ static ssize_t attach_store(struct device *dev, 
> > > struct device_attribute *attr,
> > >       spin_unlock_irqrestore(&vhci->lock, flags);
> > >       /* end the lock */
> > > +    if (usbip_event_happened(&vdev->ud)) {
> > > +        /*
> > > +         * something went wrong - event handler shutting
> > > +         * the connection and doing reset - bail out
> > > +         */
> > > +        dev_err(dev, "Event happended - handler is active\n");
> > > +        return -EAGAIN;
> > > +    }
> > > +
> >
> > detach_store() can queue shutdown event as soon as reaching "/* end 
> > the lock */" line
> > but attach_store() might be preempted immediately after verifying that
> > usbip_event_happened() was false (i.e. at this location) in order to 
> > wait for
> > shutdown event posted by detach_store() to be processed.

Please don't review code that isn't sent upstream. This repo you are
looking at is a private branch created just to verify fixes on syzbot.

I understand the race window you are talking about. I have my way of
working to resolve it.

thanks,
-- Shuah