* 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.

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...) \

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...) \

On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@baylibre.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.

          Linus

* Ingo Molnar <mingo@kernel.org> 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?

I also reviewed our other similar macros in <linux/cleanup.h>:

 - scoped_guard() appears to be single-statement safe: it uses a for() 
   statement with a partial body with an open 'else' branch, so if this 
   macro is used within single statements the entire block will be part 
   of the 'else' statement.

 - scoped_cond_guard(): similar construct to scoped_guard().

 - The other remaining multi-statement macros are variable definition 
   macros (DEFINE_CLASS(), et al), which are typically used in file 
   scope or in header scope, and are not expected to be used in single 
   statements.

So it appears to me we should be OK wrt. this class of bugs?

Thanks,

	Ingo