linux-kernel - Re: [PATCH] printk: inject caller information into the body of message

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: <20180928085648.GC1160@jagdpanzerIV>
Date:   Fri, 28 Sep 2018 17:56:48 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitriy Vyukov <dvyukov@...gle.com>,
        kbuild test robot <fengguang.wu@...el.com>,
        syzkaller <syzkaller@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] printk: inject caller information into the body of
 message

On (09/19/18 20:02), Tetsuo Handa wrote:
> I'm inclined to propose a simple one shown below, similar to just having
> several "struct cont" for concurrent printk() users.

Tetsuo, thanks for the patch.

> What Linus has commented is that implicit context is bad, and below one
> uses explicit context.
> After almost all users are converted to use below one, we might be able
> to get rid of KERN_CONT support.

The good thing about cont buffer is that we flush it on panic. E.g.
core/arch early boot stage can do:

	pr_cont("going to call early_init_foo()...");
	early_init_foo();
	pr_cont("OK\n");

should early_init_foo() panic the system we will have
"going to call early_init_foo()" on the serial console. This can
be addressed if you'd iterate printk_buffers[] in flush_on_panic().

> +#define MAX_PRINTK_BUFFERS 16
> +static struct printk_buffer printk_buffers[MAX_PRINTK_BUFFERS];

Well, hmm, maybe. Now can we have a problem of either too-small or too-large
MAX_PRINTK_BUFFERS. 16 buffers on a 4 CPU arm board most probably will just
waste some memory. At the same time we probably don't want to have NR_CPUS
buffers. The fallback to "regular printk" is still a bit troubling - technically
there may be cases when we don't fix anything.

So, overall, I'm not against your patch. There are some pros and cons,
however.

pr_line() patch seems to be simpler [probably] and smaller [definitely].
The only problem, as you have mentioned, is that people may miscalculate
the size of the buffer, which won't crash us or anything; people can overshot
even a LOG_LINE_MAX buffer. So probably I'm not completely sold on having
a fixed size printk_buffers[].

May be all we want at the end is to drop explicit buffer API and have just
two options in pr_line:

 DEFINE_PR_LINE()	-- 80-bytes (or 256) pr_line // implicit buffer
 DEFINE_PR_LINE_HUGE()	-- 1024-bytes pr_line        // implicit buffer

So, no explicit buffers, just "a normal" pr_line or "a huge" pr_line.
And no "normal printk" fallback; buffered printk line stays buffered.

The 80-bytes limit can be lifted to, say, 256-bytes.

Tetsuo, do you still want to have a fixed size array of printk buffers?

What do others think?

BTW, Tetsuo, I have addressed your pr_line suggestions/corrections.
Couldn't send the patch or reply to emails because I was offline for
a week due to personal reasons; but I can send it now - it does not
have DEFINE_PR_LINE_HUGE() macro. Just a previous version with
corrections which you have pointed out.

	-ss