linux-kernel - Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation

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: <CAMZ6Rq+PEZWzxNYDODq-Rz_Y8T_XEihyZKoY-MYo6bn5ATaGLQ@mail.gmail.com>
Date: Thu, 26 Jun 2025 11:47:15 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Markus Elfring <Markus.Elfring@....de>
Cc: linux-can@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
	kernel-janitors@...r.kernel.org, Chen Ni <nichen@...as.ac.cn>, 
	Marc Kleine-Budde <mkl@...gutronix.de>
Subject: Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation

On Thu. 26 Jun. 2025 at 01:47, Markus Elfring <Markus.Elfring@....de> wrote:
> > A real quick search shows me that this ucan driver is not an isolated case.
> > Here is an example:
> >
> > https://elixir.bootlin.com/linux/v6.16-rc3/source/drivers/media/rc/imon.c#L2137-L2148
>
> Thanks that you pointed another implementation detail out from
> the function “imon_find_endpoints”.

What I did here was simply to look at all the users of the
USB_ENDPOINT_DIR_MASK macro:

https://elixir.bootlin.com/linux/v6.16-rc3/C/ident/USB_ENDPOINT_DIR_MASK

and bingo, the very first user of that macro is the imon driver with a
true positive. I did not check the other drivers from the list, but
that is what I meant by the manual hunt: I believe that 15 minutes
would be enough to quickly check all those drivers. Of course, doing
it manually is a one time solution whereas adding the coccinelle
script is a long term solution. Also, I am just sharing my thoughts. I
am not trying to discourage you in any way, it is even the opposite:
such initiatives are really nice! Even if I do not participate in
these myself, I want to tell you my gratitude for your efforts!

> > But it does not seem to occur so often either. So not sure what is the best:
> > do a manual hunt
>
> Unlikely.
>
> I am unsure if such an aspect would become relevant for a code review
> by other contributors.
>
>
> >                  or write a coccinelle checker.
>
> I would find it more convenient to achieve corresponding adjustments
> to some degree with the help of such a development tool.
> I constructed scripts for the semantic patch language accordingly.
>
>
> >> Can the functions “usb_endpoint_is_bulk_in” and “usb_endpoint_is_bulk_out”
> >> be applied here?
> >> https://elixir.bootlin.com/linux/v6.16-rc3/source/include/uapi/linux/usb/ch9.h#L572-L595
> >
> > Further simplification, nice :)
> >
> > I didn't see that last one, so glad you found what seems to be the optimal solution!
> I am unsure if the check reordering would be desirable for this function implementation.

Ah, you want to confirm whether

  usb_endpoint_dir_in(ep) && usb_endpoint_xfer_bulk(ep)

is the same as

  usb_endpoint_xfer_bulk(ep) && usb_endpoint_dir_in(ep)

?

In this case, that is OK. *Mathematically speaking* we have this equivalence:

  a & b <=> b & a

In C it is roughly the same except if the expression has some
undefined behaviour. The typical example is:

  foo && foo->bar

Here, the short cut evaluation of the && operator will prevent the
undefined behaviour to occur if foo is NULL. And so, obviously,
refactoring as:

  foo->bar && foo

would be a bug. In our case, there is no undefined behaviour on the
right hand operand (I mean, if ep is NULL, the undefined behaviour
will already occur on the left hand operand). So we are totally safe
to reorder the operand here.

Yours sincerely,
Vincent Mailhol