Hi Alan,
On Friday, 20 April 2018 17:09:57 EEST Alan Stern wrote:
> On Thu, 19 Apr 2018, Laurent Pinchart wrote:
> > According to include/linux/usb/composite.h, the delayed_status field
> > should be protected by cdev->lock, which you should use here.
> >
> > I've read through the code and found out that, while all callers of
> > reset_config(), as well as usb_composite_setup_continue(), correctly take
> > the lock, it isn't taken around f->set_alt() in composite_setup(). This
> > causes the race condition. I wonder if a simpler fix wouldn't be to take
> > the lock before calling f->set_alt() and releasing it after incrementing
> > delayed_status. I am however worried that this could lead to deadlocks if
> > one of the existing set_alt() handlers calls a function that takes the
> > same lock. Another worry is that some of the .set_alt() handlers might
> > not expect to be called with interrupts disabled. This should be
> > analyzed, and I hope that Roger and/or Felipe will have some insight on
> > this.
>
> set_alt handlers generally have to disable and enable endpoints, which
> requires a process context. They cannot run with interrupts disabled.
Thank you for the information. Isn't the following code path problematic then
?
int
composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
{
...
case USB_REQ_SET_CONFIGURATION:
...
spin_lock(&cdev->lock);
value = set_config(cdev, ctrl, w_value);
spin_unlock(&cdev->lock);
...
}
static int set_config(struct usb_composite_dev *cdev,
const struct usb_ctrlrequest *ctrl, unsigned number)
{
...
/* Initialize all interfaces by setting them to altsetting zero. */
for (tmp = 0; tmp < MAX_CONFIG_INTERFACES; tmp++) {
...
result = f->set_alt(f, tmp, 0);
...
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
