On Sat, May 06, 2023 at 09:44:17AM +0900, Greg KH wrote:
> On Sat, Apr 29, 2023 at 05:47:33PM +0200, Wlodzimierz Lipert wrote:
> > --- a/drivers/usb/gadget/epautoconf.c
> > +++ b/drivers/usb/gadget/epautoconf.c
> > @@ -67,6 +67,8 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > )
> > {
> > struct usb_ep *ep;
> > + unsigned *epmap;
>
> Why "unsigned"? Didn't checkpatch complain about this?
In other words, the preferred style in the kernel is to use "unsigned
int" rather than just "unsigned". (Although there's plenty of old code
that still does it that way.)
> > if (isdigit(ep->name[2])) {
> > - u8 num = simple_strtoul(&ep->name[2], NULL, 10);
> > - desc->bEndpointAddress |= num;
> > - } else if (desc->bEndpointAddress & USB_DIR_IN) {
> > - if (++gadget->in_epnum > 15)
> > + num = simple_strtoul(&ep->name[2], NULL, 10);
> > + WARN_ON(num == 0 || num > 15);
>
> You just crashed the system if this happened if panic-on-warn is enabled :(
>
> Please never do this, if there is an issue, fix it up and handle it
> properly.
I disagree; I think this is an appropriate use of WARN_ON. It's an
example of "drivers should never do this, and if they do then weird,
unpredictable errors will occur". For these things we _want_ to have a
very visible notice that a problem needs to be fixed. (In this case it
wouldn't hurt to also print out the bad endpoint name as well, along
with the name of the UDC driver.)
If someone turns on panic-on-warn, it means they _do_ want their system
to crash when issues like this crop up. Like syzbot, for example. If
instead we just covered up the error then it would never get fixed.
Alan Stern
