Re: [PATCH] printk/nmi: Prevent deadlock when serializing NMI backtraces

[Sergey Senozhatsky]

On (06/06/18 13:15), Petr Mladek wrote:
> On Wed 2018-06-06 14:10:29, Sergey Senozhatsky wrote:
> > On (06/05/18 14:47), Petr Mladek wrote:
> > [..]
> > > Grr, the ABBA deadlock is still there. NMIs are not sent to the other
> > > CPUs atomically. Even if we detect that logbuf_lock is available
> > > in printk_nmi_enter() on some CPUs, it might still get locked on
> > > another CPU before the other CPU gets NMI.
> > 
> > Can we do something about "B"? :) I mean - any chance we can rework
> > locking in nmi_cpu_backtrace()?
> 
> I can't think of any possibility at the moment. Well, it does not mean
> that it does not exist.
> 
> The irony is that we need the extra lock in nmi_cpu_backtrace() only
> because we try to store the messages into the common log buffer.
> If we always use the per-CPU buffers in NMI then the lock would
> not cause any problems but it also won't be necessary.

Yep. I think we can come up with something. It seems that the only
problem is that we want in this particular case to avoid printk_nmi()->logbuf
and instead we want to enforce per-CPU printk_nmi buffers. Then we can
drop nmi_cpu_backtrace() spinlock, because the messages will be
serialized by printk_safe flush spin_lock. That doesn't sound like an
impossible thing to do. What am I missing?

Could you please check my follow up email?

> > >   => I suggest to revert the commit 719f6a7040f1bdaf96fcc70
> > >      "printk: Use the main logbuf in NMI when logbuf_lock is available"
> > >      for-4.18 and stable until we get a better solution.
> > 
> > Just random thoughts.
> > 
> > May be we need to revert it, but let's not "panic". I think [but don't
> > insist on it] that the patch in question is *probably* "good enough". It
> > addresses a bug report after all.
> 
> It was a problem reported by me. I found it when testing other changes.
> The patch improved the situation definitely. The question is if it is
> enough in practice.

Oh, certainly. But I was talking about 719f6a7040f1bdaf96fcc70. We
introduced that change in response to a bug report from Steven. He would
not be able to debug his kernel otherwise, because per-CPU printk_nmi was
too limited in size.
So on one hand we have the problem that you reported, which you found
while you were hammering/testing NMI printk-s [a valid report on its
own]; on the other hand we have the problem that Steven reported, which
he triggered while he was debugging the kernel. It might be the case that
Steven's problem is more likely to happen in real world. So that's why I
proposed to keep 719f6a7040f1bda for the time being [until we come up with
another fix]. I may be wrong.

	-ss