linux-kernel - Re: [RFC][PATCH v4 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]

Message-ID: <20160317003450.GA538@swordfish>
Date:	Thu, 17 Mar 2016 09:34:50 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Byungchul Park <byungchul.park@....com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Jan Kara <jack@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jan Kara <jack@...e.com>, Petr Mladek <pmladek@...e.com>,
	Tejun Heo <tj@...nel.org>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH v4 1/2] printk: Make printk() completely async

On (03/16/16 19:34), Byungchul Park wrote:
[..]
> > -- if we are going to have wake_up_process() in wake_up_klogd_work_func(),
> > then we need `in_sched' message to potentially trigger a recursion chain
> > 
> > wake_up_klogd_work_func()->wake_up_process()->printk()->wake_up_process()->printk()...
> > 
> > to break this printk()->wake_up_process()->printk(), we need wake_up_process() to
> > be under the logbuf lock; so vprintk_emit()'s if (logbuf_cpu == this_cpu) will act.
> 
> I am curious about how you make the wake_up_process() call and I may want
> to talk about it at the next spin. Anyway, then we will lose the last
> message when "if (logbuf_cpu == this_cpu)" acts. Is it acceptible?

yes, this is how it is. "BUG: recent printk recursion!" will be printed
instead of the message.

> IMHO it's not a good choice to use wake_up() and friend within a printk()
> since it can additionally cause another recursion. Of course, it does not
> happen if the condition (logbuf_cpu == this_cpu) acts. But I don't think
> it's good to rely on the condition with losing a message. Anyway I really
> really want to see your next spin and talk.

the alternative is NOT significantly better. pending bit is checked in
IRQ, so one simply can do

	local_irq_save();
	while (xxx) printk();
	local_irq_restore();

and _in the worst case_ nothing will be printed to console until IRQ are
enabled on this CPU. (there are some 'if's, but the worst case is just
like this. http://marc.info/?l=linux-kernel&m=145734549308803).

I'd probably prefer to add wake_up_process() to vprintk_emit() and do it
under the logbuf lock. first, we don't suffer from disabled IRQs on current
CPU, second we have somewhat better chances to break printk() recursion
*in some cases*.

> > -- if we are going to have wake_up_process() in console_unlock(), then
> > 
> > console_unlock()->{up(), wake_up_process()}->printk()->{console_lock(), console_unlock()}->{up(), wake_up_process()}->printk()...
> > 
> 
> This cannot happen. console_lock() cannot continue because the prior
> console_unlock() does not release console_sem.lock yet when
> wake_up_process() is called. Only a deadlock exists. And my patch solves
> the problem so that the deadlock cannot happen.

ah, we lost in patches. I was talking about yet another patch
(you probably not aware of. you were not Cc'd. Sorry!) that
makes console_unlock() asynchronous:

http://marc.info/?l=linux-kernel&m=145750373530161

s/wake_up/wake_up_process/ is at the end of console_unlock().

while the patch belongs to another series, I still wanted to outline it
here, since we were talking about printk() recursion.

	-ss