linux-kernel - Re: [PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

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: <bb7608cc-4a83-0e1d-0124-656246ec4a1f@linaro.org>
Date:   Tue, 9 Mar 2021 17:39:20 -0600
From:   Alex Elder <elder@...aro.org>
To:     subashab@...eaurora.org, stranche@...eaurora.org,
        davem@...emloft.net, kuba@...nel.org
Cc:     sharathv@...eaurora.org, bjorn.andersson@...aro.org,
        evgreen@...omium.org, cpratapa@...eaurora.org,
        David.Laight@...LAB.COM, elder@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C
 bit-fields

On 3/9/21 6:48 AM, Alex Elder wrote:
> Version 3 of this series uses BIT() rather than GENMASK() to define
> single-bit masks.  It then uses a simple AND (&) operation rather
> than (e.g.) u8_get_bits() to access such flags.  This was suggested
> by David Laight and really prefer the result.  With Bjorn's
> permission I have preserved his Reviewed-by tags on the first five
> patches.

Nice as all this looks, it doesn't *work*.  I did some very basic
testing before sending out version 3, but not enough.  (More on
the problem, below).

		--> I retract this series <--

I will send out an update (version 4).  But I won't be doing it
for a few more days.

The problem is that the BIT() flags are defined in host byte
order.  But the values they're compared against are not always
(or perhaps, never) in host byte order.

I regret the error, and will do a complete set of testing on
version 4 before sending it out for review.

					-Alex

> Version 2 fixed bugs in the way the value written into the header
> was computed.
> 
> The series was first posted here:
>    https://lore.kernel.org/netdev/20210304223431.15045-1-elder@linaro.org/
> Below is a summary of the original description.
> 
> This series converts data structures defined in <linux/if_rmnet.h>
> so they use integral field values with bitfield masks rather than
> relying on C bit-fields.
>    - The first three patches lay the ground work for the others.
>        - The first adds endianness notation to a structure.
>        - The second simplifies a bit of complicated code.
>        - The third open-codes some macros that needlessly
>          obscured some simple code.
>    - Each of the last three patches converts one of the structures
>      defined in <linux/if_rmnet.h> so it no longer uses C bit-fields.
> 
>      					-Alex
> 
> Alex Elder (6):
>    net: qualcomm: rmnet: mark trailer field endianness
>    net: qualcomm: rmnet: simplify some byte order logic
>    net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros
>    net: qualcomm: rmnet: use field masks instead of C bit-fields
>    net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
>    net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
> 
>   .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 11 ++--
>   .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 12 ----
>   .../qualcomm/rmnet/rmnet_map_command.c        | 11 +++-
>   .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 60 ++++++++---------
>   include/linux/if_rmnet.h                      | 65 +++++++++----------
>   5 files changed, 70 insertions(+), 89 deletions(-)
>