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: <20210310002731.adinf2sgzeshkjqd@skbuf>
Date:   Wed, 10 Mar 2021 02:27:31 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Alex Elder <elder@...aro.org>
Cc:     subashab@...eaurora.org, stranche@...eaurora.org,
        davem@...emloft.net, kuba@...nel.org, 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

Hi Alex,

On Tue, Mar 09, 2021 at 05:39:20PM -0600, Alex Elder wrote:
> 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.

I think I understand some of your pain. I had a similar situation trying
to write a driver for hardware with very strange bitfield organization,
and my top priority was actually maintaining a set of bitfield definitions
that could be taken directly from the user manual of said piece of
hardware (and similar to you, I dislike C bitfields). What I came up
with was an entirely new API called packing() which is described here:
https://www.kernel.org/doc/html/latest/core-api/packing.html
It doesn't have any users except code added by me (some in Ethernet fast
paths), and it has some limitations (mainly that it only has support for
u64 CPU words), but on the other hand, it's easy to understand, easy to
use, supports any bit/byte layout under the sun, doesn't suffer from
unaligned memory access issues due to its byte-by-byte approach, and is
completely independent of host endianness.
That said, I'm not completely happy with it because it has slightly
higher overhead compared to typical bitfield accessors. I've been on the
fence about even deleting it, considering that it's been two years since
it's in mainline and it hasn't gained much of a traction. So I would
rather try to work my way around a different API in the sja1105 driver.

Have you noticed this API and decided to not use it for whatever reason?
Could you let me know what that was? Even better, in your quest to fix
the rmnet driver, have you seen any API that is capable of extracting a
bitfield that spans two 64-bit halves of an 128 bit word in a custom bit
layout?