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: <20190906124211.2dionk2kzcslaotz@pathway.suse.cz>
Date:   Fri, 6 Sep 2019 14:42:11 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     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>,
        John Ogness <john.ogness@...utronix.de>,
        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 Fri 2019-09-06 11:06:27, Peter Zijlstra wrote:
> On Thu, Sep 05, 2019 at 04:31:18PM +0200, Peter Zijlstra wrote:
> > So I have something roughly like the below; I'm suggesting you add the
> > line with + on:
> > 
> >   int early_vprintk(const char *fmt, va_list args)
> >   {
> > 	char buf[256]; // teh suck!
> > 	int old, n = vscnprintf(buf, sizeof(buf), fmt, args);
> > 
> > 	old = cpu_lock();
> > +	printk_buffer_store(buf, n);
> > 	early_console->write(early_console, buf, n);
> > 	cpu_unlock(old);
> > 
> > 	return n;
> >   }
> > 
> > (yes, yes, we can get rid of the on-stack @buf thing with a
> > reserve+commit API, but who cares :-))
> 
> Another approach is something like:
> 
> DEFINE_PER_CPU(int, printk_nest);
> DEFINE_PER_CPU(char, printk_line[4][256]);
> 
> int vprintk(const char *fmt, va_list args)
> {
> 	int c, n, i;
> 	char *buf;
> 
> 	preempt_disable();
> 	i = min(3, this_cpu_inc_return(printk_nest) - 1);
> 	buf = this_cpu_ptr(printk_line[i]);
> 	n = vscnprintf(buf, 256, fmt, args);
> 
> 	c = cpu_lock();
> 	printk_buffer_store(buf, n);
> 	if (early_console)
> 		early_console->write(early_console, buf, n);
> 	cpu_unlock(c);
> 
> 	this_cpu_dec(printk_nest);
> 	preempt_enable();
> 
> 	return n;
> }
> 
> Again, simple and straight forward (and I'm sure it's been mentioned
> before too).
> 
> We really should not be making this stuff harder than it needs to be
> (and anybody whining about lines longer than 256 characters can just go
> away, those are unreadable anyway).

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.

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.

3. console_lock works in tty as a big kernel lock. I do not know
   much details. But people familiar with the code said that
   it was a disaster. I assume that tty is still rather
   important console. I am not sure how it would fit into the
   simple approach.

4. The console handling has got non-synchronous (console_trylock)
   quite early (ver 2.4.10, year 2001). The reason was to do not
   serialize CPUs by the speed of the console.

   Serialized output could remove many troubles. The logic in
   console_unlock() is really crazy. It might be acceptable
   for debugging. But is it acceptable on production systems?

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.

6. One huge nightmare is panic() and code called from there.
   It is a maze of hacks, including arch-specific code, to
   prevent deadlocks and get the messages out.

   Any lock might be blocked on any CPU at the moment. Or it
   it might become blocked when CPUs are stopped by NMI.

   Fully lock-less log buffer might save us some headache.
   I am not sure whether a single lock shared between printk()
   writers and console drivers will make the situation easier
   or more complicated.

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.

I would be really happy when we could ignore some of the problems
or find an easy solution. I just want to make sure that we take
into account all the known aspects.

I am sure that we could do better than we do now. I do not want
to block any improvements. I am just a bit lost in the many
black corners.

Best Regards,
Petr