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]

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

On 2018/10/09 1:03, Petr Mladek wrote:
> On Mon 2018-10-08 19:31:58, Tetsuo Handa wrote:
>>   A structure named "struct printk_buffer" is introduced for buffering
>>   up to LOG_LINE_MAX bytes of printk() output which did not end with '\n'.
>>
>>   A caller is allowed to allocate/free "struct printk_buffer" using
>>   kzalloc()/kfree() if that caller is in a location where it is possible
>>   to do so.
>>
>>   A macro named "DEFINE_PRINTK_BUFFER()" is defined for allocating
>>   "struct printk_buffer" from the stack memory or in the .bss section.
>>
>>   But since sizeof("struct printk_buffer") is nearly 1KB, it might not be
>>   preferable to allocate "struct printk_buffer" from the stack memory.
>>   In that case, a caller can use best-effort buffering mode. Two functions
>>   get_printk_buffer() and put_printk_buffer() are provided for that mode.
>>
>>   get_printk_buffer() tries to assign a "struct printk_buffer" from
>>   statically preallocated array. It returns NULL if all static
>>   "struct printk_buffer" are in use.
>>
>>   put_printk_buffer() flushes and releases the "struct printk_buffer".
>>   put_printk_buffer() must match corresponding get_printk_buffer() as with
>>   rcu_read_unlock() must match corresponding rcu_read_lock().
> 
> One problem with this API is when it is used in more complicated code
> and put_printk_buffer() is not called in some path. I mean leaking.
> We might get out of buffers easily.

Then, as an debugging config option for statically preallocated buffers,
we could record how get_printk_buffer() was called, like lockdep records
where a lock was taken.

> 
> A solution might be to store some information about the owner and
> put the buffer also when a non-buffered printk is called from
> the same context.
> 
> It might even make it easier to use. If we are able to guess the
> buffer by the context, we do not need to pass it as an argument.

It would be nice if we can omit passing "struct printk_buffer" argument.
But that results in "implicit contexts" which Linus has rejected
( https://lkml.kernel.org/CA+55aFx+5R-vFQfr7+Ok9Yrs2adQ2Ma4fz+S6nCyWHY_-2mrmw@mail.gmail.com ).

> 
> Well, I would like to avoid having the buffer connected with CPU.
> It would require to disable preemption in get_printk_buffer().
> IMHO, it would be a unintuitive and even unwanted side effect.

get_printk_buffer() is connected with the context who called "struct printk_buffer".
There is no need to disable preemption.

> 
> Best Regards,
> Petr
> 
> 
> PS: I am sorry for the late reply. I was busy with some other
> important stuff. I still have to think more about it and look
> mode deeply into the implementation.

No problem. Thank you for replying.

> 
> In each case, we need to be careful about the design.
> The API has to be easy and safe to use. Also the implementation
> should not complicate the printk design too much.
> 
> It looks promising. Also there is a high chance that it would
> be much more straightforward than the current code around
> the cont buffer ;-)
> 

We could eventually remove "struct cont" buffer.