linux-kernel - Re: [PATCH v10 1/2] printk: Make printk() completely async

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]

Date:   Fri, 2 Sep 2016 16:58:08 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Jan Kara <jack@...e.cz>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.com>, Tejun Heo <tj@...nel.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Byungchul Park <byungchul.park@....com>, vlevenetz@...sol.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v10 1/2] printk: Make printk() completely async

On (09/01/16 10:58), Petr Mladek wrote:
> On Wed 2016-08-31 21:52:24, Sergey Senozhatsky wrote:
> > On (08/31/16 11:38), Petr Mladek wrote:
> > >   2. Potential deadlocks when calling wake_up_process() by
> > >      async printk and console_unlock().
> > 
> >     * there are many reasons to those recursive printk() calls -- some
> > can be addressed, some cannot. for instance, it doesn't matter how many
> > per-CPU buffers we use for alternative printk() once the logbuf_lock is
> > corrupted.
> 
> Yup and BTW: Peter Zijlstra wants to avoid zapping locks whenever
> possible because it corrupts the state. It might solve the actual
> state but it might cause deadlock by the double unlock.

yes, don't really want to zap_locks() either.

[..]
> Great catch! From the already mentioned solutions, I would prefer
> using deferred variants of WARN()/BUG()/printk() on these locations.
> Together with using lockdep to find these locations.

hmm... need to think more. one of the problems is that we would have to
periodically "scan" for new WARNs/BUGs/etc doing all the types of random
.configs

> Also there is the Peter Zijlstra's idea of using a lockless
> "early" console to debug the situations where it happens.
> It might make sense to make such a console easy to use.

aha, not really familiar with early console.

> I am unable to find any other generic solution that would prevent this
> from the printk() side at the moment.
> 
> >     5. not 100% guaranteed printing on panic
[..]
> That might be very hard to solve in general as well. Again the PeterZ's
> idea with the lockless console might help here.

"need to google it".

> > > I wonder how to separate the problems and make them more manageable.
> > 
> > so I was thinking for a moment about doing the recursion detection rework
> > before the async_printk. just because better recursion detection is a nice
> > thing to have in the first place and it probably may help us catching some
> > of the surprises that async_printk might have. but it probably will be more
> > problematic than I thought.
> > 
> > then async_printk. I have a refreshed series on my hands, addressing
> > Viresh's reports. it certainly makes things better, but it doesn't
> > eliminate all of the lockups/etc sources.
> 
> We must separate historical possible lockups and new regressions.
> Only regressions should block the async printk series. Old
> bugs should be fixed separately to keep the series manageable.

agree.

> Anyway, I think that the async printk will make sense even
> when we solve all the other issues. If async printk does not
> cause regressions, why not make it in.

sure.

> > a console_unlock() doing
> > wake_up_process(printk_kthread) would make it better.
> 
> I am not sure what you mean by this.

I meant that this thing

	local_irq_save() // or preempt_disable()
	...
	if (console_trylock())
		console_unlock();
	...
	local_irq_restore() // or preempt_enable()


can easily lockup the system if console_trylock() was successful and there
are enough messages to print. printk_kthread can't help, because here we
basically enforce the `old' behavior. we have async printk, but not async
console output. tweaking console_unlock() to offload the actual printing loop
to printk_kthread would make the entire console output async:

	static void console_sync_flush_and_unlock(void)
	{
		for (;;) {
		...
			call_console_drivers();
		...
		}
	}

	void console_unlock(void)
	{
		if (!MOTORMOUTH && can_printk_async()) {
			up();
			wake_up_process(printk_kthread);
			return;
		}
		console_sync_flush_and_unlock();
	}

	-ss