linux-kernel - Re: [PATCH v2] printk: Relocate wake_klogd check close to the end of console

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: <20180210073333.GC485@tigerII.localdomain>
Date:   Sat, 10 Feb 2018 16:33:33 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v2] printk: Relocate wake_klogd check close to the end of
 console_unlock()

Hello,

On (02/09/18 11:39), Petr Mladek wrote:
> On Fri 2018-02-09 12:28:30, Sergey Senozhatsky wrote:
> > On (02/08/18 17:48), Petr Mladek wrote:
> > By postponing klogd wakeup we don't really address logbuf_lock
> > contention. We have no guarantees that no new printk will come
> > while klogd is active. Besides, consoles don't really delay
> > klogd - I tend to ignore the impact of msg_print_text(), it should
> > be fast. We call console drivers outside of logbuf_lock scope, so
> > everything should fine (tm).
> 
> First, I am all for waking klogd earlier.
> 
> Second, sigh, I do not have much experience with kernel performace issues.
> It is very likely that we really do not need to mind about it.

I really don't think we need to bother.
klogd loggers are as important as consoles, we need to run them anyway.

> > Basically,
> > - if consoles are suspended, we also "suspend" user space klogd.
> >   Does it really make sense?
> > 
> > - if console_lock is acquired by a preemptible task and that task
> >   is getting scheduled out for a long time (OOM, etc) then we postpone
> >   user space logging for unknown period of time. First the console_lock
> >   will have to flush pending messages and only afterwards it will wakeup
> >   klogd. Does it really make sense?
> > 
> > - If current console_lock owner jumps to retry (new pending messages
> >   were appended to the logbuf) label, user space klogd wakeup is getting
> >   postponed even further.
> > 
> > So, the final question is - since there in only one legitimate way
> > (modulo user space writes to kmsg) to append new messages to the
> > logbuf, shall we put klogd wakeup there? IOW, to vprintk_emit().
> 
> Good points! In fact, if we wakeup klogd before calling consoles,
> we would need to do it for every new message. Otherwise, klogd
> processes might miss new messages that are added in parallel
> when console_lock is taken.
[..]
> Then klogd might start sleeping while new messages are still comming

Loggers sleep when `user->seq == log_next_seq' or when
`syslog_seq == log_next_seq'. We need to just wakeup them.
Once woken up they will check the condition again, if there
are no new messages, they will schedule.

> Now, your proposed solution looked fine. Just note that we do not need
> seen_seq. vprintk_emit() knows when log_next_seq is increased.
> It would always wake klogd in this case.

Probably can wake_up_klogd() unconditionally.

> Anyway, I think about slightly different way that would prevent
> scheduling klogd intead of the vprintk_emit() caller. The trick
> is to do the wakeup with preemtion disabled. I mean something like:
[..]
> @@ -1899,9 +1899,13 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 */
>  		preempt_disable();
>  		/*
> +		 * Wake klogd with disabled preemtion so that they can run
> +		 * in parallel but they could not delay console_handling.
> +		 */
> +		wake_up_klogd();
> +		/*
>  		 * Try to acquire and then immediately release the console
> -		 * semaphore.  The release will print out buffers and wake up
> -		 * /dev/kmsg and syslog() users.
> +		 * semaphore.  The release will print out buffers.
>  		 */
>  		if (console_trylock_spinning())
>  			console_unlock();
[..]

It doesn't wakeup loggers for printk_deferred() output this way.
I want to wakeup them for every new logbuf message.

If logger preempts current printk() that it's for 1 message only.
printk() will return back and finish printing. If there will be another
printk()-s from other CPUs, then one of those CPUs will print messages
to the consoles - CPU that woke up logger had not acquired console_sem
before it was preempted by logger.

	-ss