linux-kernel - Re: [PATCH] kernel.h: Add for_each

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: <871scbwfd4.fsf@notabene.neil.brown.name>
Date:   Tue, 10 Jul 2018 20:32:23 +1000
From:   NeilBrown <neilb@...e.com>
To:     Daniel Vetter <daniel@...ll.ch>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        LKML <linux-kernel@...r.kernel.org>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Wei Wang <wvw@...gle.com>, Stefan Agner <stefan@...er.ch>,
        Andrei Vagin <avagin@...nvz.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Yisheng Xie <ysxie@...mail.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH] kernel.h: Add for_each_if()

On Tue, Jul 10 2018, Daniel Vetter wrote:

> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@...ll.ch> wrote:
>> 
>> > To avoid compilers complainig about ambigious else blocks when putting
>> > an if condition into a for_each macro one needs to invert the
>> > condition and add a dummy else. We have a nice little convenience
>> > macro for that in drm headers, let's move it out. Subsequent patches
>> > will roll it out to other places.
>> > 
>> > The issue the compilers complain about are nested if with an else
>> > block and no {} to disambiguate which if the else belongs to. The C
>> > standard is clear, but in practice people forget:
>> > 
>> > if (foo)
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> 
>> um, yeah, don't do that.  Kernel coding style is very much to do
>> 
>> 	if (foo) {
>> 		if (bar)
>> 			/* something */
>> 		else
>> 			/* something else
>> 	}
>> 
>> And if not doing that generates a warning then, well, do that.
>> 
>> > The same can happen in a for_each macro when it also contains an if
>> > condition at the end, except the compiler message is now really
>> > confusing since there's only 1 if:
>> > 
>> > for_each_something()
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> > 
>> > The for_each_if() macro, by inverting the condition and adding an
>> > else, avoids the compiler warning.
>> 
>> Ditto.
>> 
>> > Motivated by a discussion with Andy and Yisheng, who want to add
>> > another for_each_macro which would benefit from for_each_if() instead
>> > of hand-rolling it.
>> 
>> Ditto.
>> 
>> > v2: Explain a bit better what this is good for, after the discussion
>> > with Peter Z.
>> 
>> Presumably the above was discussed in whatever-thread-that-was.
>
> So there's a bunch of open coded versions of this already in kernel
> headers (at least the ones I've found). Not counting the big pile of
> existing users in drm. They are all wrong and should be reverted to a
> plain if? That why there's a bunch more patches in this series.
>
> And yes I made it clear in the discussion that if you sprinkle enough {}
> there's no warning, should have probably captured this here.
>
> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
> can stop bothering with this.

I think is it problematic to have macros like

#define for_each_foo(...) for (......) if (....)

because
   for_each_foo(...)
      if (x) ....; else ......;

is handled badly.
So in that sense, your work seems like a good thing.

However it isn't clear to me that you need a new macro.
The above macro could simply be changed to

#define for_each_foo(...) for (......) if (!....);else

Clearly people don't always think to do this, but would adding a macro
help people to think?

If we were to have a macro, it isn't clear to me that for_each_if() is a
good name.
Every other macro I've seen that starts "for_each_" causes the body to
loop.  This one doesn't.  If someone doesn't know what for_each_if()
does and sees it in code, they are unlikely to jump to the right
conclusion.
I would suggest that "__if" would be a better choice.  I think most
people would guess that means "like 'if', but a bit different", which is
fairly accurate.

I think the only sure way to avoid bad macros being written is to teach
some static checker to warn about any macro with a dangling "if".
Possibly checkpatch.pl could do that (but I'm not volunteering).

I do agree that it would be good to do something, and if people find
for_each_fi() to actually reduce the number of poorly written macros,
then I don't object to it.

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)