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

[Ingo Molnar <mingo@kernel.org>]

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