linux-kernel - Re: [PATCH] headers/cleanup.h: Remove the if_not

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: <ffebd9e3-3b5b-4fad-94ad-8eb1e15cc63b@baylibre.com>
Date: Fri, 6 Dec 2024 09:31:50 -0600
From: David Lechner <dlechner@...libre.com>
To: Ingo Molnar <mingo@...nel.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
 Thomas Gleixner <tglx@...utronix.de>, Will Deacon <will@...nel.org>,
 Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
 Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility

On 12/6/24 3:19 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
>> On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@...libre.com> wrote:
>>>
>>>         cond_guard(mutex_intr, &st->lock, &ret);
>>>         if (ret)
>>>                 return ret;
>>
>> I'm not convinced that improves on anything.
>>
>> You just replace one disgusting syntax with another, and force people
>> to have a variable that they may not want to have (even if they have
>> an error return variable, it might commonly be an error pointer, for
>> example)
>>
>> I really think the basic issue is that "cond_guard" itself is a pretty
>> broken concept. It simply doesn't work very well in the C syntax.
>>
>> I wish people just gave up on it entirely rather than try to work
>> around that fundamental fact.
>>
>> Not that long ago, Mathieu wanted to introduce "inactive guards" for
>> some similar reasons - kind of "conditional guards, except the
>> conditional is outside the guard". And I pointed out that the fix was
>> to rewrite the disgusting code so that THEY WEREN'T NEEDED in the
>> place he wanted to use them. Rewriting things to "Just Don't Do That,
>> Then" actually just improved code entirely:
>>
>>    https://lore.kernel.org/all/CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@mail.gmail.com/
>>
>> and honestly, I suspect the same is often true of this whole
>> "if_not_guard()" thing. It's not *hugely* often needed, and I strongly
>> suspect that the explicitly scoped version would be a *lot* safer.
>>
>> The "if_not_guard()" model may be great for mindless conversions of
>> existing code. But I'm not convinced it's a great interface in itself,
>> or that "mindless conversions" of conditional locking is actually a
>> good thing.
> 
> Ok, agreed - and to progress with fixing the bug & the fragility you 
> noticed, let's remove if_cond_guard() as a first step via the patch 
> below.
> 
> Thanks,
> 
> 	Ingo
> 
> =================================>
> From: Ingo Molnar <mingo@...nel.org>
> Date: Fri, 6 Dec 2024 10:13:32 +0100
> Subject: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility
> 
> Linus noticed that the new if_not_guard() definition is fragile:
> 
>    "This macro generates actively wrong code if it happens to be inside an
>     if-statement or a loop without a block.
> 
>     IOW, code like this:
> 
>       for (iterate-over-something)
>           if_not_guard(a)
>               return -BUSY;
> 
>     looks like will build fine, but will generate completely incorrect code."
> 
> The reason is that the __if_not_guard() macro is multi-statement, so
> while most kernel developers expect macros to be simple or at least
> compound statements - but for __if_not_guard() it is not so:
> 
>  #define __if_not_guard(_name, _id, args...)            \
>         BUILD_BUG_ON(!__is_cond_ptr(_name));            \
>         CLASS(_name, _id)(args);                        \
>         if (!__guard_ptr(_name)(&_id))
> 
> To add insult to injury, the placement of the BUILD_BUG_ON() line makes
> the macro appear to compile fine, but it will generate incorrect code
> as Linus reported, for example if used within iteration or conditional
> statements that will use the first statement of a macro as a loop body
> or conditional statement body.
> 
> It doesn't appear to be possible to turn this macro into a robust
> single or compound statement that could be used in single statements,
> due to the necessity to define an auto scope variable with an open
> scope and the necessity of it having to expand to a partial 'if'
> statement with no body.
> 
> Instead of trying to work around this fragility, just remove the
> construct before it gets used by code.
> 
> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> Cc: David Lechner <dlechner@...libre.com>
> ---
I agree this is the right thing to do. Thanks for writing up the patch.

Acked-by: David Lechner <dlechner@...libre.com>