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 15:14:46 -0500
From:   Jason Wessel <jason.wessel@...driver.com>
To:     Doug Anderson <dianders@...omium.org>,
        Arnd Bergmann <arnd@...nel.org>
Cc:     Daniel Thompson <daniel.thompson@...aro.org>,
        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 3/22/21 2:22 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Mar 22, 2021 at 11:19 AM Arnd Bergmann <arnd@...nel.org> wrote:
>>
>> On Mon, Mar 22, 2021 at 6:07 PM Doug Anderson <dianders@...omium.org> wrote:
>>> On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <arnd@...nel.org> wrote:
>>>>
>>>> -#define v1printk(a...) do { \
>>>> -       if (verbose) \
>>>> -               printk(KERN_INFO a); \
>>>> -       } while (0)
>>>> -#define v2printk(a...) do { \
>>>> -       if (verbose > 1) \
>>>> -               printk(KERN_INFO a); \
>>>> -               touch_nmi_watchdog();   \
>>>> -       } while (0)
>>>> -#define eprintk(a...) do { \
>>>> -               printk(KERN_ERR a); \
>>>> -               WARN_ON(1); \
>>>> -       } while (0)
>>>> +#define v1printk(a...) do {            \
>>>
>>> nit: In addition to the indentation change you're also lining up the
>>> backslashes. Is that just personal preference, or is there some
>>> official recommendation in the kernel? I don't really have a strong
>>> opinion either way (IMO each style has its advantages).
>>
>> I don't think there is an official recommendation, I just think the
>> style is more common, and it helped my figure out what the
>> indentation should look like in this case.
> 
> OK, makes sense. I just wasn't sure if there was some standard that I
> wasn't aware of. Given that you have to touch all these lines anyway
> then making them all pretty like this seems fine to me.
> 
> 
>>>> +       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.
>>
>> No idea. It was like this in Jason's original version from 2008.
> 
> Yeah, I noticed the same. I'd be curious what Daneil (or Jason if he's
> reading) says. I suppose i could always wait until your patch lands
> and then send a new patch that puts it inside the "if" statement and
> we can debate it then.
> 


The original board this was developed with was a 32bit eeepc.

The intent was that when v2printk() was called for a verbose > 1
condition the touch_nmi_watchdog() was called.   The test case
where a whole lot of single steps are executed sequentially was not
letting the watchdog get reset by the normal kernel routine.
The serial port was so slow it was pretty easy to hit this problem
and it would just power cycle itself.

The original intent would have bee:

#define v2printk(a...) do {		\
	if (verbose > 1) {		\
		printk(KERN_INFO a);	\
		touch_nmi_watchdog();	\
	}				\
} while (0)


I'd guess this probably not the first time gcc-11 is finding brace
imbalances.

Cheers,
Jason.