Re: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility

[David Lechner <dlechner@baylibre.com>]

On 11/20/24 5:36 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>>  - <linux/cleanup.h>:
>>>     - Add if_not_cond_guard() conditional guard helper (David Lechner)
>>
>> I've pulled this, but I'm unhappy.
>>
>> 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.
>>
>> Honestly, just switching the order of the BUILD_BUG_ON() and the
>> CLASS() declaration looks like it would have fixed this (because then
>> the '_id' won't be in scope of the subsequent if-statement any more),
>> but I'm unhappy with how apparently nobody even bothered to think
>> about such a fundamental issue with macros.
>>
>> Macros that expand to statements absolutely *ALWAYS* need to deal with
>> "what if we're in a single-statement situation?"
> 
> How about the fix below?
> 
> Thanks,
> 
> 	Ingo
> 
> =======================>
> From: Ingo Molnar <mingo@kernel.org>
> Date: Wed, 20 Nov 2024 11:56:31 +0100
> Subject: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility
> 
> 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.
> 
> While 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 - we can at least make sure the macro won't 
> build if used in a single-statement construct: such as by making the 
> CLASS() line the first statement in the macro, followed by the other 
> statements, which would break the build, as the single statement would 
> close the scope.

Here is another option:

We could scrap this macro and try a different approach completely.
Instead we could create something that works a bit different but is
actually a single C statement.

Instead of this code...

	if_not_guard(mutex_intr, &st->lock)
		return -EINTR;

We could write this...

	int ret;
	
	cond_guard(mutex_intr, &st->lock, &ret);
	if (ret)
		return ret;

In this case, the cond_guard() macro would expand to a single statement,
namely a variable declaration statement.

This would also fix another thing that bugged me about the existing
scoped_cond_guard() that this is aiming to replace. scoped_cond_guard()
swallows the return value of the acquire function and just returns a
handle or NULL, possibly losing information.

In fact, mutex_lock_interruptible() that I used in this example can
return -EINTR, -EALREADY, or -EDEADLK. This means that patches like
[1] are actually unintentionally changing behavior because instead of
passing on the return value, they assume that only -EINTR could be
returned and hard-code that. This will cause bugs if anyone higher up
the call stack that is checking for a specific error code. If we want
to fix if_cond_guard() we should make it robust against this mistake
as well. But at this point, I think reverting my patch and going
back to the drawing board is the best option.

[1]: https://lore.kernel.org/all/20240904043104.1030257-2-dmitry.torokhov@gmail.com/


Note: We can't make the most obvious macro that works like this...

	ret = cond_guard(mutex_intr, &st->lock);

for the same reason that if_not_guard() is destined to be buggy - there
just isn't a way to do it in a single statement/expression while keeping
the cleanup variable declaration in the current scope. For this reason
I am proposing the next best thing where ret is an output parameter.

> 
> Do this.
> 
> To test this, I added an artificial if_not_guard() usecase within a 
> single statement:
> 
> Before:
> 
> 	$ make kernel/ptrace.o
> 	CC      kernel/ptrace.o
> 	$
> 
> After:
> 
> 	CC      kernel/ptrace.o
> 	In file included from ./include/linux/irqflags.h:17,
> 		       from ./arch/x86/include/asm/special_insns.h:10,
> 		       from ./arch/x86/include/asm/processor.h:25,
> 		       from ./include/linux/sched.h:13,
> 		       from kernel/ptrace.c:13:
> 	kernel/ptrace.c: In function ‘ptrace_attach’:
> 	./include/linux/cleanup.h:258:9: error: expected expression before ‘class_mutex_intr_t’
> 
> I'd also like to note that the original submission by David Lechner did 
> not contain the BUILD_BUG_ON() line, so it was safer than what we ended 
> up committing. Mea culpa.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Fixes: 36c2cf88808d cleanup: Add conditional guard helper
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/cleanup.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 966fcc5ff8ef..263f14085617 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -351,8 +351,8 @@ _label:									\
>  	__scoped_cond_guard(_name, _fail, __UNIQUE_ID(label), args)
>  
>  #define __if_not_guard(_name, _id, args...)		\
> -	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
>  	CLASS(_name, _id)(args);			\
> +	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
>  	if (!__guard_ptr(_name)(&_id))
>  
>  #define if_not_guard(_name, args...) \