linux-kernel - Re: [RFC][PATCHv4 4/6] printk: report lost messages in printk safe/nmi contexts

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Dec 2016 13:50:13 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Tejun Heo <tj@...nel.org>,
        Calvin Owens <calvinowens@...com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Laura Abbott <labbott@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCHv4 4/6] printk: report lost messages in printk
 safe/nmi contexts

On Thu 2016-12-01 11:10:42, Sergey Senozhatsky wrote:
> On (11/25/16 12:07), Petr Mladek wrote:
> [..]
> > > +static void report_message_lost(atomic_t *num_lost, char *fmt)
> > > +{
> > > +	int lost = atomic_xchg(num_lost, 0);
> > > +
> > > +	if (lost) {
> > > +		char msg[56];
>
> > I would really like to avoid a hard coded buffer size. Such things
> > are likely to bite us in the future.
> 
> why would scnprintf() overflow.

Sure, it will not overflow. But still, such a small hard coded buffer
size looks too hacky to me.


> > I thought about reshuffling a lot of logic, adding more wrappers,
> > ... But the solution might be easy in the end, see below.
> > 
> > > +
> > > +		scnprintf(msg, sizeof(msg), fmt, lost);
> > > +
> > > +		printk_safe_flush_line(msg, strlen(msg));
> > 
> > This made my brain spin a lot. I wondered if it did what we wanted
> > and it was safe.
> > 
> > On one hand, it is supposed to work because use exactly this
> > function in __printk_safe_flush() where you call this from.
> > 
> > One question is if it does what we want in different contexts.
> > Let's look at it:
> > 
> > 1. It calls printk_deferred() in NMI context. There is a risk
> >    of a deadlock. But it is called only from
> >    printk_safe_flush_on_panic() which is the last resort. Therefore
> >    it does exactly what we want.
> > 
> > 2. It calls printk()->printk_func()->vprintk_emit() in normal context.
> >    It is what we want in normal context.
> > 
> > 3. It calls printk()->printk_func()->v printk_safe() in printk_safe
> >    context. This does not look correct. IMHO, this might happen
> >    only printk_safe_flush_on_panic() and we want to use
> >    printk_deferred() here as well.
> [..]
> > The easiest solution would be to simply call printk_deferred()
> > here. Everything will be deferred after the async printk() patchset
> > anyway.
> > 
> > I would even use printk_deferred() in printk_safe_flush_line()
> > for each context. It is not optimal but it works very well
> > and it makes the code much more straightforward.
> 
> yes, good point.
> we can call deferred printk for anything there; or replace that in_nmi()
> check with the `printk_safe_context != 0' one, and then route the message
> via printk or printk_deferred.

Yup, it might be an option and sounds good.

Anyway, I would use printk_deferred() to print the warnings about lost
messages. It is perfectly fine and you will not need the hard coded
temporary buffer.

Best regards,
Petr