Re: [PATCH] x86: Avoid pr_cont() in show_opcodes()

[Ingo Molnar <mingo@kernel.org>]

* Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> From: Tetsuo Handa <penguin-kernel@I-love.SKAURA.ne.jp>
> 
> Since syzbot is confused by concurrent printk() messages [1],
> this patch changes show_opcodes() to use snprintf().
> 
> When we start adding prefix to each line of printk() output,
> we will be able to handle concurrent printk() messages.
> 
> [1] https://syzkaller.appspot.com/text?tag=CrashReport&x=139d342c400000
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SKAURA.ne.jp>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/kernel/dumpstack.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 666a284..bb47426 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -97,22 +97,24 @@ void show_opcodes(u8 *rip, const char *loglvl)
>  	u8 opcodes[OPCODE_BUFSIZE];
>  	u8 *ip;
>  	int i;
> -
> -	printk("%sCode: ", loglvl);
> +	int pos = 0;
> +	char buf[(3 * OPCODE_BUFSIZE + 2) + 1];
>  
>  	ip = (u8 *)rip - code_prologue;
>  	if (probe_kernel_read(opcodes, ip, OPCODE_BUFSIZE)) {
> -		pr_cont("Bad RIP value.\n");
> +		printk("%sCode: Bad RIP value.\n", loglvl);
>  		return;
>  	}
>  
>  	for (i = 0; i < OPCODE_BUFSIZE; i++, ip++) {
>  		if (ip == rip)
> -			pr_cont("<%02x> ", opcodes[i]);
> +			pos += snprintf(buf + pos, sizeof(buf) - pos,
> +					"<%02x> ", opcodes[i]);
>  		else
> -			pr_cont("%02x ", opcodes[i]);
> +			pos += snprintf(buf + pos, sizeof(buf) - pos,
> +					"%02x ", opcodes[i]);
>  	}
> -	pr_cont("\n");
> +	printk("%sCode: %s\n", loglvl, buf);

Does this change the output?

- If yes, could you show the before/after output in the changelog,

- If not (i.e. if only the number of printk calls is changed, the output is the 
  same), could you say so in the changelog?

Also, 3*OPCODE_BUFSIZE+2+1 is 195 bytes - isn't that a bit too much on-stack 
footprint?

Thanks,

	Ingo