| Message ID | 20220308171818.384491-1-jakobkoschel@gmail.com |
|---|---|
| Headers | show |
| Series | usb: gadget: remove usage of list iterator past the loop | expand |
On 08/03/2022 18:18, Jakob Koschel wrote: > If the list representing the request queue does not contain the expected > request, the value of the list_for_each_entry() iterator will not point > to a valid structure. To avoid type confusion in such case, the list > iterator scope will be limited to the list_for_each_entry() loop. > > In preparation to limiting scope of the list iterator to the list traversal > loop, use a dedicated pointer to point to the found request object [1]. > > Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/ > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > --- > drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c > index 89f1f8c9f02e..bf803e013458 100644 > --- a/drivers/usb/gadget/udc/s3c-hsudc.c > +++ b/drivers/usb/gadget/udc/s3c-hsudc.c > @@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > { > struct s3c_hsudc_ep *hsep = our_ep(_ep); > struct s3c_hsudc *hsudc = hsep->dev; > - struct s3c_hsudc_req *hsreq; > + struct s3c_hsudc_req *hsreq = NULL, *iter; > unsigned long flags; > > hsep = our_ep(_ep); > @@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > > spin_lock_irqsave(&hsudc->lock, flags); > > - list_for_each_entry(hsreq, &hsep->queue, queue) { > - if (&hsreq->req == _req) > - break; > + list_for_each_entry(iter, &hsep->queue, queue) { > + if (&iter->req != _req) > + continue; > + hsreq = iter; > + break; You have in the loop both continue and break, looks a bit complicated. Why not simpler: if (&iter->req == _req) { hsreq = iter; break; } ? Less code, typical (expected) code flow when looking for something in the list. Is your approach related to some speculative execution? > } > - if (&hsreq->req != _req) { > + if (!hsreq) { > spin_unlock_irqrestore(&hsudc->lock, flags); > return -EINVAL; > } Best regards, Krzysztof
This patch set removes any use of the list iterator variable past the list body. This will allow defining the list iterator variable within the list_for_each_entry_*() macros to avoid any (invalid) use after the loop. If no break/goto was hit during list traversal the list iterator variable would otherwise be a bogus pointer since it is computed on something that is not actually an element of the list. I've basically followed what we discussed in: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/ There are some cases where it might be possible to 'ditch' the tmp variable and refactor the code past the loop into the loop body. For the sake of keeping the *_dequeue() functions more similar, I've decided against doing it for some and leaving it in others. In general there are four use cases after the loop body here: 1) using &req->req in a comparision after the loop 2) using the iterator as a pointer in a comparision after the loop 3) use the &iterator->list to compare with the head to see if the loop exits early 4) using the iterator past the loop but using the rc variable to see if the loop exits early Change since v1: - renamed list iterator variable from 'tmp' to 'iter' (Linus) - inverted the conditions within the loop (Linus) - reverted the check on 'rc' after the loop (Greg & Krzysztof) Jakob Koschel (26): usb: gadget: fsl: remove usage of list iterator past the loop body usb: gadget: bdc: remove usage of list iterator past the loop body usb: gadget: udc: atmel: remove usage of list iterator past the loop body usb: gadget: udc: pxa25x: remove usage of list iterator past the loop body usb: gadget: udc: at91: remove usage of list iterator past the loop body usb: gadget: goku_udc: remove usage of list iterator past the loop body usb: gadget: udc: gr_udc: remove usage of list iterator past the loop body usb: gadget: lpc32xx_udc: remove usage of list iterator past the loop body usb: gadget: mv_u3d: remove usage of list iterator past the loop body usb: gadget: udc: mv_udc_core: remove usage of list iterator past the loop body usb: gadget: net2272: remove usage of list iterator past the loop body usb: gadget: udc: net2280: remove usage of list iterator past the loop body usb: gadget: omap_udc: remove usage of list iterator past the loop body usb: gadget: s3c-hsudc: remove usage of list iterator past the loop body usb: gadget: udc-xilinx: remove usage of list iterator past the loop body usb: gadget: aspeed: remove usage of list iterator past the loop body usb: gadget: configfs: remove using list iterator after loop body as a ptr usb: gadget: legacy: remove using list iterator after loop body as a ptr usb: gadget: udc: max3420_udc: remove using list iterator after loop body as a ptr usb: gadget: tegra-xudc: remove using list iterator after loop body as a ptr usb: gadget: composite: remove check of list iterator against head past the loop body usb: gadget: pxa27x_udc: replace usage of rc to check if a list element was found usb: gadget: composite: remove usage of list iterator past the loop body usb: gadget: udc: core: remove usage of list iterator past the loop body usb: gadget: dummy_hcd: remove usage of list iterator past the loop body usb: gadget: udc: s3c2410: remove usage of list iterator past the loop body drivers/usb/gadget/composite.c | 36 +++++++++++++----------- drivers/usb/gadget/configfs.c | 24 +++++++++------- drivers/usb/gadget/legacy/hid.c | 23 +++++++-------- drivers/usb/gadget/udc/aspeed-vhub/epn.c | 12 ++++---- drivers/usb/gadget/udc/at91_udc.c | 12 ++++---- drivers/usb/gadget/udc/atmel_usba_udc.c | 13 +++++---- drivers/usb/gadget/udc/bdc/bdc_ep.c | 13 ++++++--- drivers/usb/gadget/udc/core.c | 20 +++++++------ drivers/usb/gadget/udc/dummy_hcd.c | 17 +++++------ drivers/usb/gadget/udc/fsl_qe_udc.c | 13 +++++---- drivers/usb/gadget/udc/fsl_udc_core.c | 13 +++++---- drivers/usb/gadget/udc/goku_udc.c | 12 ++++---- drivers/usb/gadget/udc/gr_udc.c | 12 ++++---- drivers/usb/gadget/udc/lpc32xx_udc.c | 12 ++++---- drivers/usb/gadget/udc/max3420_udc.c | 18 +++++++----- drivers/usb/gadget/udc/mv_u3d_core.c | 12 ++++---- drivers/usb/gadget/udc/mv_udc_core.c | 12 ++++---- drivers/usb/gadget/udc/net2272.c | 13 +++++---- drivers/usb/gadget/udc/net2280.c | 13 +++++---- drivers/usb/gadget/udc/omap_udc.c | 12 ++++---- drivers/usb/gadget/udc/pxa25x_udc.c | 13 +++++---- drivers/usb/gadget/udc/pxa27x_udc.c | 13 +++++---- drivers/usb/gadget/udc/s3c-hsudc.c | 12 ++++---- drivers/usb/gadget/udc/s3c2410_udc.c | 17 +++++------ drivers/usb/gadget/udc/tegra-xudc.c | 12 ++++---- drivers/usb/gadget/udc/udc-xilinx.c | 13 +++++---- 26 files changed, 227 insertions(+), 165 deletions(-) base-commit: 719fce7539cd3e186598e2aed36325fe892150cf -- 2.25.1