linux-kernel - Re: [RFC][PATCHv5 3/7] printk: introduce per-cpu safe

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:   Mon, 12 Dec 2016 14:54:18 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     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>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [RFC][PATCHv5 3/7] printk: introduce per-cpu safe_print seq
 buffer

On Sat 2016-12-10 12:10:22, Sergey Senozhatsky wrote:
> On (12/09/16 17:46), Petr Mladek wrote:
> > > -/*
> > > - * Safe printk() for NMI context. It uses a per-CPU buffer to
> > > - * store the message. NMIs are not nested, so there is always only
> > > - * one writer running. But the buffer might get flushed from another
> > > - * CPU, so we need to be careful.
> > > - */
> > 
> > We should keep/create a good description here because the function
> > has a non-trivial code. What about something like?
> > 
> 
> which is really not related to this patch set.

I am sorry but I do not understand. This patch removes description
that explained constrains of a rather complex code. In fact, the
constrains has changed because we started using the function also
in other context. When will be the right time/patchset to explain
it?

> 
> > >  	 * Make sure that all old data have been read before the buffer was
> > > @@ -261,14 +263,95 @@ void printk_safe_flush_on_panic(void)
> > >  	printk_safe_flush();
> > >  }
> > >  
> > > +#ifdef CONFIG_PRINTK_NMI
> > > +/*
> > > + * Safe printk() for NMI context. It uses a per-CPU buffer to
> > > + * store the message. NMIs are not nested, so there is always only
> > > + * one writer running. But the buffer might get flushed from another
> > > + * CPU, so we need to be careful.
> > > + */
> > 
> > Hmm, I wanted to describe why we need another per-CPU buffer in NMI
> > and I am not sure that we really need it.
> 
> NMI-printk can interrupt safe-printk's vsnprintf() in the middle of
> the "while (*fmt)" loop: safe-priNMI-PRINTK

But this already happens when any of the WARNs is triggered
inside vsnprintf(). Either this is safe or we are in
trouble.

Well, there is a difference. NMI can come at anytime and vsnprintf()
continues printing the original string once we are back from NMI.
But if we hit WARN() inside vsnprintf(), it usually means an error,
vsnprintf() stops printing into the given buffer and returns. It
means that it does not overwrite the message printed by the nested
printks.

The only exceptions are WARN_ONCE() calls in set_field_width()
and set_precision(). They are self-repairing, vsnprintf()
continues printing and will overwrite the nested warnings.
Well, I am not sure if we should bother.

By other words, we really need separate per-CPU buffer for NMI
and the generic printk_safe. I am sorry for the noise.

Well, is it that bad to ask for better comments? You see that
I ended in quite some doubts, even found problems, when I tried
to review the code carefully. Or am I dumb and it was all obvious?

Best Regards,
Petr

PS: I know that I am sometimes in too nitpicking mode and it
might be annoying and discouraging. I have to find the right
boundaries.