linux-kernel - Re: [PATCH next v4 1/2] lib/dump_stack: move cpu lock to printk.c

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: <877dirb4t2.fsf@jogness.linutronix.de>
Date:   Fri, 18 Jun 2021 17:01:37 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH next v4 1/2] lib/dump_stack: move cpu lock to printk.c

On 2021-06-17, Steven Rostedt <rostedt@...dmis.org> wrote:
> Can we add this lock to early_printk() ?
>
> This would make early_printk() so much more readable.
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 421c35571797..2b749c745c1f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2259,6 +2259,7 @@ struct console *early_console;
>  
>  asmlinkage __visible void early_printk(const char *fmt, ...)
>  {
> +	unsigned long flags;
>  	va_list ap;
>  	char buf[512];
>  	int n;
> @@ -2270,7 +2271,9 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
>  	n = vscnprintf(buf, sizeof(buf), fmt, ap);
>  	va_end(ap);
>  
> +	printk_cpu_lock_irqsave(flags);
>  	early_console->write(early_console, buf, n);
> +	printk_cpu_unlock_irqrestore(flags);
>  }
>  #endif

Since the cpu lock is also taken in NMI context (for example, via
nmi_cpu_backtrace()/dump_stack()), the main concerns are:

1. locks that are taken by a CPU that is holding the cpu lock

2. NMI contexts that take any type of lock

(Actually, #2 is just a special case of #1 where an NMI interrupted a
task that was holding the cpu lock.)

For early_printk() the early USB devices look to be a
problem. early_xdbc_write() will take a spinlock. Assuming the
early_console was also registered as a normal console (via "keep") we
could end up in the following deadlock between the normal console and
early_printk() writes:

    CPU0                          CPU1
    ----                          ----
    early_printk()                console->write()
      cpu_lock()                    spinlock()
      early_console->write()      *NMI*
        spinlock()                cpu_lock()

The upcoming atomic console work addresses this by implementing a new
write_atomic() callback that is lockless (and SMP-safe) or aware of the
cpu lock to avoid dead locks such as above.

AFAICT, the USB devices (CONFIG_EARLY_PRINTK_USB) are the only
early_printk() candidates that use locking. So for all other
early_printk() implementations I think your suggestion would work fine.

Although, in general, early_printk() is not SMP-safe. So I'm not sure
how much safety we need to include at this point.

John Ogness