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

[Ingo Molnar <mingo@kernel.org>]

* 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