Messages in this thread | | | From | Zheyu Ma <> | Date | Sun, 22 May 2022 13:06:59 +0800 | Subject | Re: [PATCH v2] char: xillybus: Check endpoint type before allocing |
| |
On Fri, May 20, 2022 at 1:41 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > 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.
Please correct me if I'm wrong. I think the driver needs to check if the urb has the correct type before submitting the urb, and this check should be done early. This driver uses endpoint_alloc() to allocate an endpoint and then uses this endpoint for transfers. And one of the arguments of endpoint_alloc() is 'ep_num'. So I want to iterate over the endpoints and exclude the unwanted address as follows:
if (ep->bEndpointAddress != ep_num) continue;
> 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.
I care about the address of the endpoint because it is an argument of endpoint_alloc(). I don't know a better solution to check the endpoint even if I know the address.
Thanks, Zheyu Ma
| |