On Wed, Oct 19, 2016 at 04:41:40PM +0200, Petr Mladek wrote:
> On Tue 2016-10-18 19:08:31, Peter Zijlstra wrote:
> > Some people figured vprintk_emit() makes for a nice API and exported
> > it, bypassing the kdb trap.
> >
> > This still leaves vprintk_nmi() outside of the kbd reach, should that
> > be fixed too?
>
> Good question! vkdb_printf() tries to avoid a deadlock but the code is racy:
>
> int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> {
> [...]
> /* Serialize kdb_printf if multiple cpus try to write at once.
> * But if any cpu goes recursive in kdb, just print the output,
> * even if it is interleaved with any other text.
> */
> if (!KDB_STATE(PRINTF_LOCK)) {
> KDB_STATE_SET(PRINTF_LOCK);
> spin_lock_irqsave(&kdb_printf_lock, flags);
> got_printf_lock = 1;
> atomic_inc(&kdb_event);
> } else {
> __acquire(kdb_printf_lock);
> }
>
>
> Let's have the following situation:
>
> CPU1 CPU2
>
> if (!KDB_STATE(PRINTF_LOCK)) {
> KDB_STATE_SET(PRINTF_LOCK);
>
> if (!KDB_STATE(PRINTF_LOCK)) {
> } else {
> __acquire(kdb_printf_lock);
> }
>
> Now, both CPUs are in the critical section and happily writing over each
> other, e.g. in
>
> vsnprintf(next_avail, size_avail, fmt, ap);
>
> I quess that we want to fix this race. But I am not sure if it will
> be done an NMI-safe way. I am going to send a patch for this.
Something like patch 3 in this series should do I suppose. But the
vkdb_printf() thing using spin_lock_irqsave() seems to suggest it was
never meant to be used from NMI context.
