linux-kernel - Re: [PATCH 25/26] usb: gadget: dummy_hcd: replace usage of rc to check if a list element was found

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <024FB75A-A6AD-4ED1-8E1D-5126A4692FD8@gmail.com>
Date:   Sun, 6 Mar 2022 20:16:46 +0100
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Felipe Balbi <balbi@...nel.org>, Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Cristian Birsan <cristian.birsan@...rochip.com>,
        Al Cooper <alcooperx@...il.com>, Li Yang <leoyang.li@....com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Daniel Mack <daniel@...que.org>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Michal Simek <michal.simek@...inx.com>,
        "open list:USB GADGET/PERIPHERAL SUBSYSTEM" 
        <linux-usb@...r.kernel.org>, Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH 25/26] usb: gadget: dummy_hcd: replace usage of rc to
 check if a list element was found



> On 6. Mar 2022, at 19:00, Greg KH <gregkh@...uxfoundation.org> wrote:
> 
> On Sun, Mar 06, 2022 at 06:50:33PM +0100, Jakob Koschel wrote:
>> To move the list iterator variable into the list_for_each_entry_*()
>> macro in the future it should be avoided to use the list iterator
>> variable after the loop body.
>> 
>> To *never* use the list iterator variable after the loop it was
>> concluded to use a separate iterator variable [1].
>> 
>> This removes the need to check the rc value to determine if the
>> break/goto was hit and can be made more obvious
>> by checking if the variable was set within the list traversal loop.
>> 
>> Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/
>> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
>> ---
>> drivers/usb/gadget/udc/dummy_hcd.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
>> index a2d956af42a2..f21944707707 100644
>> --- a/drivers/usb/gadget/udc/dummy_hcd.c
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -751,7 +751,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> 	struct dummy		*dum;
>> 	int			retval = -EINVAL;
>> 	unsigned long		flags;
>> -	struct dummy_request	*req = NULL;
>> +	struct dummy_request	*req = NULL, *tmp;
>> 
>> 	if (!_ep || !_req)
>> 		return retval;
>> @@ -763,17 +763,18 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> 
>> 	local_irq_save(flags);
>> 	spin_lock(&dum->lock);
>> -	list_for_each_entry(req, &ep->queue, queue) {
>> -		if (&req->req == _req) {
>> -			list_del_init(&req->queue);
>> +	list_for_each_entry(tmp, &ep->queue, queue) {
>> +		if (&tmp->req == _req) {
>> +			list_del_init(&tmp->queue);
>> 			_req->status = -ECONNRESET;
>> +			req = tmp;
>> 			retval = 0;
>> 			break;
>> 		}
>> 	}
>> 	spin_unlock(&dum->lock);
>> 
>> -	if (retval == 0) {
>> +	if (req) {
> 
> There's no need for this change as we are testing retval, not req here,
> unlike the other udc drivers.
> 
> So this one I think is correct as-is, or am I mistaken somehow?

The check is correct as-is. I just felt it would be more explicit to
actually check if the pointer that is used within the block is not
NULL than implicitly checking this through retval. There are other
blocks which do like:

list_for_each_entry(pos, head, list) {
	if (...) {
		rc = -1;
		goto fail;
	}
}

rc = call_unrelated_function(...);
if (rc == -1)
	goto fail;

...

return 0;

fail:
	*pos->member;

While this code is obviously broken and then one in this patch works fine,
I feel like it's easier to follow the rule of always checking of pos != NULL.

It might also make it easier for some static analyzers to find potential
NULL pointer dereferences but it probably doesn't matter.

If you prefer keeping retval I'll just do that instead.

> 
> thanks,
> 
> greg k-h

Jakob