Re: [PATCH] twist: allow converting pr_devel()/pr_debug() into printk(KERN_DEBUG)

[Petr Mladek]

On Mon 2020-05-25 19:43:04, Tetsuo Handa wrote:
> On 2020/05/25 17:42, Petr Mladek wrote:
> > I see few drawbacks with this patch:
> > 
> > 1. It will cause adding much more messages into the logbuffer even
> >    though they are not flushed to the console. It might cause that
> >    more important messages will get overridden before they reach
> >    console. They might also make hard to read the full log.
> 
> Since the user of this twist option will select console loglevel in a way
> KERN_DEBUG messages are not printed to consoles, KERN_DEBUG messages will
> be immediately processed (and space for future messages will be reclaimed).
> Therefore, I don't think that more important messages will get overridden.

This is not fully true. More important messages will still be printed
to the console. The debug messages will not be skipped before the
older messages are proceed.

I mean that many debug messages might cause losing more important ones
before the old important messages are proceed.


> This twist option might increase possibility of mixing KERN_DEBUG messages
> and non-KERN_DEBUG messages due to KERN_CONT case.
> 
> But if these concerns turn out to be a real problem, we can redirect
> pr_devel()/pr_debug() to simple snprintf() which evaluates arguments
> but discards the result without storing into the logbuffer.
> 
> > 
> > 2. Crash inside printk() causes recursive messages. They are currently
> >    printed into the printk_safe() buffers and there is a bigger risk
> >    that they will not reach the console.
> 
> Currently "static char textbuf[LOG_LINE_MAX];" is "static" because it is used
> under logbuf_lock. If we remove "static", we can use "char textbuf[LOG_LINE_MAX];"
> without logbuf_lock. Then, we can bring potentially dangerous-and-slow vscnprintf()
> in vprintk_store() to earlier stage (and vprintk_store() will need to do simple
> copy) so that oops in printk() will happen before entering printk-safe context.
> I think that this change follows a direction which lockless logbuf will want.

No, LOG_LINE_MAX is too big to be allocated on stack.

Well, it would be possible to call vsprintf() with NULL buffer. It is
normally used to calculate the length of the message before it is
printed. But it also does all the accesses without printing anything.


> > Have you tested this patch by the syzcaller with many runs, please?
> > Did it helped to actually discover more bugs?
> > Did it really made things easier?
> 
> syzbot can't test with custom patches. The only way to test this patch is
> to send to e.g. linux-next.git which syzbot is testing.

OK, we could try this via some test branch that will go into
linux-next but it would not be scheduled for the next merge window.

For the testing, this patch might be good enough.

For eventual upstreaming, I would prefer to handle this in
lib/dynamic_debug.c by enabling all entries by default. This
would solve all DYNAMIC_DEBUG_BRANCH() users at one place.

Anyway, I would like to see a proof that it really helped to find
some bugs an easier way before upstreaming.

Best Regards,
Petr