Re: [PATCH v2] panic: add an option to replay all the printk message in buffer

[Petr Mladek]

On Wed 2019-04-17 14:48:44, Feng Tang wrote:
> Hi Andrew,
> 
> On Tue, Apr 16, 2019 at 09:19:22PM -0700, Andrew Morton wrote:
> > On Wed, 10 Apr 2019 23:37:18 +0800 Feng Tang <feng.tang@xxxxxxxxx> wrote:
> > 
> > > Currently on panic, kernel will lower the loglevel and print out
> > > new printk msg only with console_flush_on_panic().
> > > 
> > > Add an option for users to configure the "panic_print" to see
> > > all dmesg in buffer, some of which they may have never seen due
> > > to the loglevel setting, which will help debugging too.
> > > 
> > > Thanks to Petr Mladek as somes codes come directly from the sample
> > > code in his review comments.
> > 
> > CONFIG_PRINTK=n:
> > 
> > kernel/printk/printk.c: In function console_unlock:
> > kernel/printk/printk.c:2419:11: warning: __builtin_memcpy writing 27 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
> >     len += sprintf(text + len,
> >            ^~~~~~~~~~~~~~~~~~~
> >             "Replaying the entire log:\n");
> >             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > because LOG_LINE_MAX=0 and PREFIX_MAX=0.
> 
> Thanks for catching this!
> 
> > 
> > 
> > Which is interesting.  The pre-existing
> > 
> > 			len = sprintf(text,
> > 				      "** %llu printk messages dropped **\n",
> > 				      log_first_seq - console_seq);
> > 
> > in console_unlock() has the same issue, but the compiler doesn't seem
> > to want to warn.
> 
> For this one, I did some check, and it should be related with the
> conditional check 
> 	if (console_seq < log_first_seq) {
> 
> Both the console_seq and log_first_seq will not be touched by any code
> when CONFIG_PRINTK=n, and compiler will simply skip the whole code block,
> as "console_seq < log_first_seq" will never happen.
> 
> But code block following "if (console_replay)" will be compiled, that's
> why these warning message will be shown.
> 
> > 
> > (Also, using sprintf() is a bit lame for the new message - could use
> > strlcpy()).
> > 
> > I'll drop the patch for now - we don't want that warning to come out. 
> > console_unlock() needs some fixing for the CONFIG_PRINTK=n case.
> 
> My instant thought would be put the console_unlcok() and similar funcs
> under CONFIG_PRINTK protection, while adding nop functions in the "else"
> segment.
> 
> But complexer question will be when CONFIG_PRINTK=n, how those console_xxx
> functions should consider these to make compiled binary smaller (though it
> rarely happens). would wait for Petr/Sergey/Steven's insights.

I guess that it is because console_sem is historically used to
synchronize some unrelated things, espcially in tty code.
Unfortunately, it is not easy to clean this up.

For this patch, the best solution seems to be using scnprintf()
instead of sprintf().

Please, note the difference between snprintf() and scnprintf().
We need the "scn*" variant the returns the number of really
printed characters.

Best Regards,
Petr