linux-kernel - Re: [RFC PATCH v4 0/9] printk: new ringbuffer implementation

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: <87mufhqbyf.fsf@linutronix.de>
Date:   Sat, 07 Sep 2019 00:47:20 +0200
From:   John Ogness <john.ogness@...utronix.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Petr Mladek <pmladek@...e.com>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v4 0/9] printk: new ringbuffer implementation

On 2019-09-06, Peter Zijlstra <peterz@...radead.org> wrote:
>> I wish it was that simple. It is possible that I see it too
>> complicated. But this comes to my mind:
>> 
>> 1. The simple printk_buffer_store(buf, n) is not NMI safe. For this,
>>    we might need the reserve-store approach.
>
> Of course it is, and sure it has a reserve+commit internally. I'm sure
> I posted an implenentation of something like this at some point.
>
> It is lockless (wait-free in fact, which is stronger) and supports
> multi-readers. I'm sure I posted something like that before, and ISTR
> John has something like that around somewhere too.

Yes. It was called RFCv1[0].

> The only thing I'm omitting is doing vscnprintf() twice, first to
> determine the length, and then into the reservation. Partly because I
> think that is silly and 256 chars should be plenty for everyone,
> partly because that avoids having vscnprintf() inside the cpu_lock()
> and partly because it is simpler to not do that.

Yes, this approach is more straight forward and was suggested in the
feedback to RFCv1. Although I think the current limit (1024) should
still be OK. Then we have 1 dedicated page per CPU for vscnprintf().

>> 2. The simple approach works only with lockless consoles. We need
>>    something else for the rest at least for NMI. Simle offloading
>>    to a kthread has been blocked for years. People wanted the
>>    trylock-and-flush-immediately approach.
>
> Have an irq_work to wake up a kthread that will print to shit
> consoles.

This is the approach in all the RFC versions.

>> 5. John planed to use the cpu_lock in the lockless consoles.
>>    I wonder if it was only in the console->write() callback
>>    or if it would spread the lock more widely.

The 8250 driver in RFCv1 uses the cpu-lock in console->write() on a
per-character basis and in console->write_atomic() on a per-line
basis. This is necessary because the 8250 driver cannot run lockless. It
requires synchronization for its UART_IER clearing/setting before/after
transmit.

IMO the existing early console implementations are _not_ safe for
preemption. This was the reason for the new write_atomic() callback in
RFCv1.

> Right, I'm saying that since you need it anyway, lift it up one layer.
> It makes everything simpler. More simpler is more better.

This was my reasoning for using the cpu-lock in RFCv1. Moving to a
lockless ringbuffer for RFCv2 was because there was too much
resistance/concern surrounding the cpu-lock. But yes, if we want to
support atomic consoles, the cpu-lock will still be needed.

The cpu-lock (and the related concerns) were discussed here[1].

>> 7. People would complain when continuous lines become less
>>    reliable. It might be most visible when mixing backtraces
>>    from all CPUs. Simple sorting by prefix will not make
>>    it readable. The historic way was to synchronize CPUs
>>    by a spin lock. But then the cpu_lock() could cause
>>    deadlock.
>
> Why? I'm running with that thing on, I've never seen a deadlock ever
> because of it.

As was discussed in the thread I just mentioned, introducing the
cpu-lock means that _all_ NMI functions taking spinlocks need to use the
cpu-lock. Even though Peter has never seen a deadlock, a deadlock is
possible if a BUG is triggered while one such spinlock is held. Also
note that it is not allowed to have 2 cpu-locks in the system. This is
where the BKL references started showing up.

Spinlocks in NMI context are rare, but they have existed in the past and
could exist again in the future. My suggestion was to create the policy
that any needed locking in NMI context must be done using the one
cpu-lock.

John Ogness

[0] https://lkml.kernel.org/r/20190212143003.48446-1-john.ogness@linutronix.de
[1] https://lkml.kernel.org/r/20190227094655.ecdwhsc2bf5spkqx@pathway.suse.cz