Re: [PATCH v2] char: xillybus: Check endpoint type before allocing

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> On Sat, May 14, 2022 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, May 14, 2022 at 07:48:19PM +0800, Zheyu Ma wrote:
> > > The driver submits bulk urb without checking the endpoint type is
> > > actually bulk.
> > >
> > > [    3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> > > [    3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
> > > [    3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
> > > [    3.115318] Call Trace:
> > > [    3.115452]  <TASK>
> > > [    3.115570]  try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
> > > [    3.115838]  xillyusb_probe+0x488/0x1230 [xillyusb]
> > >
> > > Add a check in endpoint_alloc() to fix the bug.
> > >
> > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > > ---
> > > Changes in v2:
> > >     - Check the endpoint type at probe time
> > > ---
> > >  drivers/char/xillybus/xillyusb.c | 27 ++++++++++++++++++++++++++-
> > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
> > > index dc3551796e5e..4467f13993ef 100644
> > > --- a/drivers/char/xillybus/xillyusb.c
> > > +++ b/drivers/char/xillybus/xillyusb.c
> > > @@ -167,6 +167,7 @@ struct xillyusb_dev {
> > >       struct device           *dev; /* For dev_err() and such */
> > >       struct kref             kref;
> > >       struct workqueue_struct *workq;
> > > +     struct usb_interface *intf;
> > >
> > >       int error;
> > >       spinlock_t error_lock; /* protect @error */
> > > @@ -475,6 +476,25 @@ static void endpoint_dealloc(struct xillyusb_endpoint *ep)
> > >       kfree(ep);
> > >  }
> > >
> > > +static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 ep_num)
> > > +{
> > > +     struct usb_host_interface *if_desc = xdev->intf->altsetting;
> > > +     int i;
> > > +
> > > +     for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
> > > +             struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
> > > +
> > > +             if (ep->bEndpointAddress != ep_num)
> > > +                     continue;
> > > +
> > > +             if ((usb_pipein(ep_num) && usb_endpoint_is_bulk_in(ep)) ||
> > > +                     (usb_pipeout(ep_num) && usb_endpoint_is_bulk_out(ep)))
> > > +                     return 0;
> > > +     }
> >
> > Why not use the built-in usb core functions that do this for you instead
> > of hand-parsing this?  Look at usb_find_common_endpoints() and related
> > functions, that should make this much easier.
> 
> Thanks for your reminder. But in this driver, we have to check not
> only the type and direction of the endpoint, but also the address of
> it. And the endpoint's address is sometimes dynamic. For example,  in
> the function xillyusb_open():
> 
> out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> 
> However, usb_find_common_endpoints() can only find the first endpoint
> that satisfies the condition, not on a specific address. I cannot find
> a more suitable built-in core function, please correct me if I'm
> wrong.

I do not understand the problem here, it looks like your code above that
I responded to doesn't care about specific addresses at all.  It is just
walking all of them and making sure that it is a bulk in/out endpoint.

And why do you care about the address of the endpoint?  If you know
that, then there's no need to walk them all and you can check that even
easier.

thanks,

greg k-h