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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160316020115.GA3217@swordfish>
Date:	Wed, 16 Mar 2016 11:01:15 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	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,
	Byungchul Park <byungchul.park@....com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Jan Kara <jack@...e.cz>
Subject: Re: [RFC][PATCH v4 1/2] printk: Make printk() completely async

Hello Petr,

On (03/15/16 16:58), Petr Mladek wrote:
[..]
> > +static bool __read_mostly printk_sync = !IS_ENABLED(CONFIG_SMP);
> > +module_param_named(synchronous, printk_sync, bool, S_IRUGO | S_IWUSR);
> 
> If we make it writtable, we also need to handle the situation that
> it gets disabled at runtime. It means to make sure that the kthread
> will be running event printk_sync was set during the boot.

yes, I just thought this morning that may be disabling 'write' here would
be ok.

> What about this?
> 
> int need_flush_console;
> 
> 	while(1) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (!need_flush_console)
> 			schedule();
> 		__set_current_state(TASK_RUNNING);
> 
> 		need_flush_console =  false;
> 
> > +		console_lock();
> > +		console_unlock();
> > +	}

much better, indeed.
I assume `need_flush_console' is primarily for avoiding schedule() cost?
not that it closes the race window 100%, it can be false at the time we
check it, and become true by the time we schedule(). TASK_INTERRUPTIBLE
should prevent lost wake_up() case, AFAIK.

> Also I wonder if we need some special handling of the system freezing
> but I do not thing so.

hm, I don't think so either.

> > +	printk_thread = kthread_run(printing_func, NULL, "printk");
> > +	BUG_ON(IS_ERR(printk_thread));
> 
> I would prefer to force the synchronous mode instead.

ok, no strong opinion here, I thought that if the system can't create
a kthread in late_initcall(), then it probably doesn't have many chances
to survive anyway.

> > + * Delayed printk version, for scheduler-internal messages:
> 
> This is not longer related to sheduler only.

this has changed. KTHREAD/IRQ  split is not needed anymore, please
see below.

> BTW: I suggest to move this whole section in a separate patch.
> It will be more clear what has changed for the async printk
> and what stays for the deferred printk.

hm, sounds good.


> 	if (pending & PRINTK_PENDING_CONSOLE_OUTPUT) {
> 		if (printk_sync || !printk_kthread) {
> 			/* If trylock fails, someone else is doing the printing */
> 			if (console_trylock())
> 				console_unlock();
> 		} else {
> 			wake_up_process(printk_kthread);
> 		}
> 
> 	if (pending & PRINTK_PENDING_KLOGD_WAKEUP)
> 		wake_up_interruptible(&log_wait);

yes, agree. this is what I have here:
http://marc.info/?l=linux-kernel&m=145805101825604

> > +	bool in_panic = console_loglevel == CONSOLE_LOGLEVEL_MOTORMOUTH;
> > +	bool sync_print = printk_sync;
> 
> I would force the global printk_sync if we are in_panic
> 
> 	if (in_panic)
> 		printk_sync = true;

can add, yes.

> > -	/* If called from the scheduler, we can not call up(). */
> > -	if (!in_sched) {
> > +	if (sync_print) {
> >  		lockdep_off();
> 
> I wonder if it might be much easier with If we used only the two
> PRINTK_PENDING flags and force global printk_sync when in panic.

two PENDING flags stuff was my bad. (I replied here
http://marc.info/?l=linux-kernel&m=145805101825604)

in short, my intention was to move it out of that part of vprintk_emit() that
can recurse, but cannot detect the recursion. wake_up()/wake_up_process()
add spin_locks/etc., which add possibilities of
        vprint_emit()->spin_lock()->spin_dump()->vprintk_emit()->...
that will not be handled by vprintk_emit() recursion detection code. but
I guess I simply want to move this under the logbuf lock section after all,
so printk recursion detection will have better chances to help us out.

> Sigh, it would be great to rename also wake_up_klogd_work and
> wake_up_klogd_work_func(). They are not only about klogd.
> Well, this should be separate patch as well because it
> was even before.

hm, yes, as a separate patch later I think.

> I still to thing about possible races. Especially, when checking
> printk_kthread and printk_sync.

hm, I don't think we risk anything here. if CPU saw an 'old' (NULL) @printk_kthread
then it just would do direct printk. once it's !NULL, we can wake it up.
is your concern here that `pointer = VALUE' can be !atomic?

> I hope that some of the above suggestions makes sense. vprintk_emit()
> is crazy already now. I feel motivated to do not make it worse ;-)

thanks for review.

	-ss