netdev - Re: [PATCH iwl-next v2 08/13] lib: packing: fix QUIRK_MSB_ON_THE

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a150453e-6a98-4d57-938c-d499edf0d759@intel.com>
Date: Tue, 3 Sep 2024 14:03:35 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: netdev <netdev@...r.kernel.org>, Anthony Nguyen
	<anthony.l.nguyen@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-next v2 08/13] lib: packing: fix
 QUIRK_MSB_ON_THE_RIGHT behavior



On 9/2/2024 4:25 PM, Vladimir Oltean wrote:
> Hi Jacob,
> 
> It's very cool that you and Przemek (and possibly others) spent the time
> to untangle this. Thanks! Just a microscopic nitpick below.
> 
> On Wed, Aug 28, 2024 at 01:57:24PM -0700, Jacob Keller wrote:
>>   That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
>>   inverts bit offsets inside a byte.
>>
>> Essentially, the mapping for physical bit offsets should be reserved for a
> 							      ~~~~~~~~
> 							      reversed
> 

Yep.

>>   Logical: 55 54 53 52 51 50 49 48 47 45 44 43 42 41 40 39
>>                   ^  ^  ^  ^  ^  ^  ^  ^  ^
> 
> These 16 bits should have been 55-40. Bit 46 is missing, and bit 39 is
> extraneous.
> 
> Also, I honestly think that another "Byte boundary:" line would help the
> reader see the transformation proposed as an example better. Like this:
> 
>  Byte boundary: |                       |                       |
>        Logical:   55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40
>                          ^  ^  ^  ^  ^  ^  ^  ^  ^
> 

Good catch, and yea a byte boundary helps readability.

>> Fixes: 554aae35007e ("lib: Add support for generic packing operations")
> 
> When there is no user-observable issue in mainline, I believe there is
> no reason for a Fixes: tag, even if the bug is very real. My understanding
> is that the role of the tag is to help the backporting process to stable.
> Using it here could possibly confuse the maintainers that it needs to be
> backported, even though it is spelled out that it needs not be.
> 

Fair. I view the fixes tags as also helpful because it helps quickly
locate the code when I'm reviewing a past commit when trying to
understand why something was changed. I could move this to a mention in
the commit message text without an explicit fixes tag though.