linux-kernel - Re: [PATCH 2/2] printk: wake up klogd in vprintk

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:   Wed, 18 Apr 2018 16:04:09 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] printk: wake up klogd in vprintk_emit

On Sat 2018-04-14 12:01:45, Sergey Senozhatsky wrote:
> We wake up klogd very late - only when current console_sem owner
> is done pushing pending kernel messages to the serial/net consoles.
> In some cases this results in lost syslog messages, because kernel
> log buffer is a circular buffer and if we don't wakeup syslog long
> enough there are chances that logbuf simply will wrap around.
> 
> The patch moves the klog wake up call to vprintk_emit(), which is
> the only legit way for a kernel message to appear in the logbuf,
> right before we attempt to grab the console_sem (possibly spinning
> on it waiting for the hand off) and call console drivers.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
>  kernel/printk/printk.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 2f4af216bd6e..86f0b337cbf6 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1888,6 +1888,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
>  
> +	wake_up_klogd();
>  	logbuf_unlock_irqrestore(flags);

The change makes perfect sense and I am fine with the idea. I just
wonder if there is a strong reason to do the wake_up before
releasing the logbuf_lock. It makes an assumption that it needs
to be synchronized by logbuf_lock.

In fact, I would feel more comfortable if we move this to the end
of vprintk_emit() right before return printk_len. This will be
more close to the current behavior (console first). But it will
still wakeup klogd much earlier and regularly if there is
a flood of messages.

Best Regards,
Petr