On (05/17/18 16:39), Petr Mladek wrote:
>
> CPU0 CPU1 CPU2
>
> printk()
> vprintk_emit()
> spin_lock(&logbuf_lock)
>
> trigger_all_cpu_backtrace()
> raise()
>
> nmi_enter()
> printk_nmi_enter()
> if (this_cpu_read(printk_context)
> & PRINTK_SAFE_CONTEXT_MASK)
> // false
> else
> // looks safe to use printk_deferred()
> this_cpu_or(printk_context,
> PRINTK_NMI_DEFERRED_CONTEXT_MASK);
>
> nmi_cpu_backtrace()
> arch_spin_lock(&lock);
> show_regs()
>
> nmi_enter()
> nmi_cpu_backtrace()
> arch_spin_lock(&lock);
>
> printk()
> vprintk_func()
> vprintk_deferred()
> vprintk_emit()
> spin_lock(&logbuf_lock)
>
> DEADLOCK: between &logbuf_lock from vprintk_emit() and
> &lock from nmi_cpu_backtrace().
>
> CPU0 CPU1
> lock(logbuf_lock) lock(lock)
> lock(lock) lock(logbuf_lock)
>
[..]
> Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
This is a pretty cool find!
Acked-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> - if ((this_cpu_read(printk_context) & PRINTK_SAFE_CONTEXT_MASK) &&
> - raw_spin_is_locked(&logbuf_lock)) {
> + if (raw_spin_is_locked(&logbuf_lock))
> this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
> - } else {
> + else
> this_cpu_or(printk_context, PRINTK_NMI_DEFERRED_CONTEXT_MASK);
> - }
A question - can we switch to a bitwise OR?
if (this_cpu_read(printk_context) & PRINTK_SAFE_CONTEXT_MASK) ||
raw_spin_is_locked(&logbuf_lock)
just to check per-CPU `printk_context' first and only afterwards
access the global `logbuf_lock'. printk_nmi_enter() happens on
every CPU, so maybe we can avoid some overhead by checking the
local per-CPU data first.
-ss
