linux-kernel - Re: [PATCH] printk: Add best-effort printk() buffering.

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, 03 May 2017 02:30:11 -0700
From:   Joe Perches <joe@...ches.com>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        linux-kernel@...r.kernel.org
Cc:     mingo@...hat.com, peterz@...radead.org, pmladek@...e.com,
        sergey.senozhatsky@...il.com, rostedt@...dmis.org
Subject: Re: [PATCH] printk: Add best-effort printk() buffering.

On Wed, 2017-05-03 at 15:21 +0900, Tetsuo Handa wrote:
> Joe Perches wrote:
> > On Sun, 2017-04-30 at 22:54 +0900, Tetsuo Handa wrote:
> > > Sometimes we want to printk() multiple lines in a group without being
> > > disturbed by concurrent printk() from interrupts and/or other threads.
> > > For example, mixed printk() output of multiple thread's dump makes it
> > > hard to interpret.
> > > 
> > > This patch introduces fixed-sized statically allocated buffers for
> > > buffering printk() output for each thread/context in best effort
> > > (i.e. up to PAGE_SIZE bytes, up to 16 concurrent printk() users).
> > 
> > []
> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > 
> > []
> > > +#define MAX_PRINTK_BUFFERS 16
> > > +static struct printk_buffer {
> > > +	unsigned long context; /* printk_context() */
> > > +	unsigned int nested;
> > > +	unsigned int used; /* Valid bytes in buf[]. */
> > > +	char buf[PAGE_SIZE];
> > > +} printk_buffers[MAX_PRINTK_BUFFERS];
> > 
> > Perhaps these buffers could be acquired by
> > alloc_page rather than be static structures and
> > the sizeof buf[PAGE_SIZE] should be reduced by
> > 	sizeof(unsigned long) +
> > 	sizeof(unsigned int) +
> > 	sizeof(unsigned int)
> > so that struct printk_buffers is exactly
> > PAGE_SIZE.
> 
> When should the buffers be allocated? If upon boot, there will be little
> difference. If the first time each buffer is needed, we introduce a risk
> of failing to allocate memory using alloc_page(GFP_ATOMIC | __GFP_NOWARN)
> and a risk of stack overflow during alloc_page() because printk() has to be
> prepared for being called from stack-tight situation.

This is supposed to be best effort anyway.

> Also, while dynamic allocation can allow linked list of the buffer, we
> will need to introduce a lock for traversing the list, which might become
> more expensive than walking fixed-sized array of the buffer.

Shouldn't matter as this is supposed to be best effort
and any printk is already quite expensive and not fast-path.

> We could avoid list traversal by passing "struct printk_buffer" argument,
> but since there are so many functions which expect pr_cont() behavior,
> scattering "struct printk_buffer" argument is a big patch.

There would be possible to add pid instead.

> Thus, I think fixed-sized statically allocated buffers is the most
> reasonable choice. Using a CONFIG_ option for controlling how many pages
> should be allocated for "struct printk_buffer" might make sense for systems
> with little RAM.

Simpler, but not better.