linux-kernel - Re: [RFC][PATCH 1/3] printk: keep kernel cont support always enabled

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: <20181009081410.cunmqz4zc7sdhwkx@pathway.suse.cz>
Date:   Tue, 9 Oct 2018 10:14:10 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dmitriy Vyukov <dvyukov@...gle.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Tejun Heo <tj@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH 1/3] printk: keep kernel cont support always enabled

On Tue 2018-10-02 11:38:34, Sergey Senozhatsky wrote:
> Since 5c2992ee7fd8a29 ("printk: remove console flushing special
> cases for partial buffered lines") we don't print cont fragments
> to the consoles; cont lines are now proper log_buf entries and
> there is no "consecutive continuation flag" anymore: we either
> have 'c' entries that mark continuation lines without fragments;
> or '-' entries that mark normal logbuf entries. There are no '+'
> entries anymore.
> 
> However, we still have a small leftover - presence of ext_console
> drivers disables kernel cont support and we flush each pr_cont()
> and store it as a separate log_buf entry. Previously, it worked
> because msg_print_ext_header() had that "an optional external merge
> of the records" functionality:
> 
> 	if (msg->flags & LOG_CONT)
> 		cont = (prev_flags & LOG_CONT) ? '+' : 'c';
> 
> We don't do this as of now, so keep kernel cont always enabled.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
>  kernel/printk/printk.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 308497194bd4..e72cb793aff1 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -192,16 +192,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  	return 0;
>  }
>  
> -/*
> - * Number of registered extended console drivers.
> - *
> - * If extended consoles are present, in-kernel cont reassembly is disabled
> - * and each fragment is stored as a separate log entry with proper
> - * continuation flag so that every emitted message has full metadata.  This
> - * doesn't change the result for regular consoles or /proc/kmsg.  For
> - * /dev/kmsg, as long as the reader concatenates messages according to
> - * consecutive continuation flags, the end result should be the same too.
> - */
> +/* Number of registered extended console drivers. */
>  static int nr_ext_console_drivers;
>  
>  /*
> @@ -1806,12 +1797,8 @@ static void cont_flush(void)
>  
>  static bool cont_add(int facility, int level, enum log_flags flags, const char *text, size_t len)
>  {
> -	/*
> -	 * If ext consoles are present, flush and skip in-kernel
> -	 * continuation.  See nr_ext_console_drivers definition.  Also, if
> -	 * the line gets too long, split it up in separate records.
> -	 */
> -	if (nr_ext_console_drivers || cont.len + len > sizeof(cont.buf)) {
> +	/* If the line gets too long, split it up in separate records. */
> +	if (cont.len + len > sizeof(cont.buf)) {
>  		cont_flush();
>  		return false;
>  	}

Just to be sure. The original purpose was to get full information
including the metadata and dictionary via extended console drivers,
see commit 6fe29354befe4c46e ("printk: implement support for extended
console drivers").

IMHO, only the dictionary was really important but it was actually
lost:

  static void cont_flush(void)
  {
  [...]
	log_store(cont.facility, cont.level, cont.flags, cont.ts_nsec,
		  NULL, 0, cont.buf, cont.len);

Nobody noticed because the only dictionary user is dev_printk()
and dev_cont() is _not_ defined.

As a result, I think that this change will rather improve things.
Well, I wonder if we should write something of the above into
the commit message. Either way:

Reviewed-by: Petr Mladek <pmladek@...e.com>

Best Regards,
Petr