linux-kernel - Re: [PATCH v5 1/7] bits: split the definition of the asm and non-asm GENMASK()

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: <d27b4c54-a44b-4df0-8c50-0cc818be92ba@wanadoo.fr>
Date: Sat, 8 Mar 2025 00:50:20 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: David Laight <david.laight.linux@...il.com>
Cc: Yury Norov <yury.norov@...il.com>,
 Lucas De Marchi <lucas.demarchi@...el.com>,
 Rasmus Villemoes <linux@...musvillemoes.dk>,
 Jani Nikula <jani.nikula@...ux.intel.com>,
 Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
 Rodrigo Vivi <rodrigo.vivi@...el.com>, Tvrtko Ursulin
 <tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Andrew Morton <akpm@...ux-foundation.org>,
 linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, Andi Shyti <andi.shyti@...ux.intel.com>,
 David Laight <David.Laight@...LAB.COM>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v5 1/7] bits: split the definition of the asm and non-asm
 GENMASK()

On 07/03/2025 at 22:27, David Laight wrote:
> On Fri, 7 Mar 2025 18:58:08 +0900
> Vincent Mailhol <mailhol.vincent@...adoo.fr> wrote:
> 
>> On 07/03/2025 at 04:23, David Laight wrote:
>>> On Thu, 06 Mar 2025 20:29:52 +0900
>>> Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@...nel.org> wrote:
>>>   
>>>> From: Vincent Mailhol <mailhol.vincent@...adoo.fr>

(...)

>>>> +#define GENMASK(h, l)		__GENMASK(h, l)
>>>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)  
>>>
>>> What do those actually expand to now?
>>> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
>>> so the expansions of __GENMASK() and __GENMASK_ULL() contained the
>>> same numeric constants.  
>>
>> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
>>
>> But the two macros still expand to something different on 32 bits
>> architectures:
>>
>>   * __GENMASK:
>>
>>       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
>>
>>   * __GENMASK_ULL:
>>
>>       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
>>
>> On 64 bits architecture these are the same.
> 
> If the assembler is naive and uses the cpu shift instruction for the >>
> then a lot of cpu (including all x86 since the 286) mask off the high bits.
> So __GENMASK_ULL() may well generate the expected pattern - provided it
> is 32bits wide.

"If", "may", that's still a lot of conditionals in your sentence :)

I do not have enough knowledge to prove or disprove this, but what I am
sure of is that this uncertainty tells me that this is not something I
want to touch myself.

I picked up this stalled fixed width patch series and re-spinned it
because I had confidence here. I do not want to extend this series with
some asm clean-up which looks uncertain to me. I am not telling you are
wrong but just that I will happily delegate this to whoever has more
confidence than me!

>>> This means they should be generating the same values.
>>> I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
>>> My suspicion is that a 32bit assembler used 32bit signed integers and a
>>> 64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
>>> be 64bit).
>>> So the asm versions need to avoid the right shift and only do left shifts.
>>>
>>> Which probably means they need to be enirely separate from the C versions.
>>> And then the C ones can have all the ULL() removed.  
>>
>> In this v5, I already have the ULL() removed from the non-uapi C
>> version. And we are left with two distinct variants:
>>
>>   - the uapi C & asm
>>   - the non-uapi C (including fix width)
>>
>> For the uapi ones, I do not think we can modify it without a risk of
>> breaking some random userland. At least, this is not a risk I will take.
>> And if we have to keep the __GENMASK() and __GENMASK_ULL(), then I would
>> rather just reuse these for the asm variant instead of splitting further
>> more and finding ourselves with three variants:
>>
>>   - the uapi C
>>   - the asm
>>   - the non-uapi C (including fix width)
>>
>> If __GENMASK() and __GENMASK_ULL() were not in the uapi, I would have
>> agreed with you.
>>
>> If you believe that the risk of modifying the uapi GENMASK*() is low
>> enough, then you can submit a patch. But I will definitely not touch
>> these myself.
> 
> I don't think you'll break userspace by stopping the uapi .h working
> for asm files (with __ASSEMBLER__ defined).
> 
> But someone else might have a different opinion.

How do you know that there isn't someone somewhere who is using the uapi
__GENMASK*() in their userland asm code? Wouldn't such code break?

You may argue that the uapi is not meant to be used that way, but at the
end, we are not supposed to make assumptions on how the uapi code will
be used. Once it is exposed, if something break because the uapi was not
used in the intended way, it is still the kernel fault, not the userland.


Yours sincerely,
Vincent Mailhol