linux-kernel - Re: [PATCH] kgdb: fix gcc-11 warning on indentation

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]

Date:   Mon, 22 Mar 2021 19:23:13 +0000
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Arnd Bergmann <arnd@...nel.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...e.hu>,
        Christian Brauner <christian.brauner@...ntu.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kgdb: fix gcc-11 warning on indentation

On Mon, Mar 22, 2021 at 10:04:57AM -0700, Doug Anderson wrote:
> > +       if (verbose)                    \
> > +               printk(KERN_INFO a);    \
> > +} while (0)
> > +#define v2printk(a...) do {            \
> > +       if (verbose > 1)                \
> > +               printk(KERN_INFO a);    \
> > +       touch_nmi_watchdog();           \
> 
> This touch_nmi_watchdog() is pretty wonky. I guess maybe the
> assumption is that the "verbose level 2" prints are so chatty that the
> printing might prevent us from touching the NMI watchdog in the way
> that we normally do and thus we need an extra one here?
> 
> ...but, in that case, I think the old code was _wrong_ and that the
> intention was that the touch_nmi_watchdog() should only be if "verose
> > 1" as the indentation implied. There doesn't feel like a reason to
> touch the watchdog if we're not doing anything slow.

I'm not entirely sure I'd like to second guess the intent here. This
macro has been there since this file was introduced but several callers
have been added since then. We have to guess their intent too!

So, whilst I think you are probably right, v2printk() does appears in
places such as the single step test loop which makes it pretty
difficult to decide by inspection whether or not touching the watchdog
is useful.

It's something that could be further examined... but I'd be a little
reluctant to combine it directly with a whitespace change!


Daniel.