Re: [PATCH printk-rework 09/12] um: synchronize kmsg_dumper

From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org, Thomas Meyer <thomas@m3y3r.de>,
	Richard Weinberger <richard@nod.at>
Subject: Re: [PATCH printk-rework 09/12] um: synchronize kmsg_dumper
Date: Tue, 2 Feb 2021 14:26:06 +0100	[thread overview]
Message-ID: <YBlS7nYgck0TWdpY@alley> (raw)
In-Reply-To: <875z3bk9z1.fsf@jogness.linutronix.de>

On Mon 2021-02-01 17:57:14, John Ogness wrote:
> (Added CC: Thomas Meyer, Richard Weinberger)
> 
> On 2021-02-01, Petr Mladek <pmladek@suse.com> wrote:
> >> In preparation for removing printk's @logbuf_lock, dumpers that have
> >> assumed to be protected against parallel calls must provide their own
> >> synchronization. Add a locally static spinlock to synchronize the
> >> kmsg_dump call and temporary buffer usage.
> >> 
> >> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> >> ---
> >>  arch/um/kernel/kmsg_dump.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> >> index f38349ad00ea..173999422ed8 100644
> >> --- a/arch/um/kernel/kmsg_dump.c
> >> +++ b/arch/um/kernel/kmsg_dump.c
> >> @@ -1,5 +1,6 @@
> >>  // SPDX-License-Identifier: GPL-2.0
> >>  #include <linux/kmsg_dump.h>
> >> +#include <linux/spinlock.h>
> >>  #include <linux/console.h>
> >>  #include <shared/init.h>
> >>  #include <shared/kern.h>
> >> @@ -9,8 +10,10 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
> >>  				enum kmsg_dump_reason reason,
> >>  				struct kmsg_dumper_iter *iter)
> >>  {
> >> +	static DEFINE_SPINLOCK(lock);
> >>  	static char line[1024];
> >>  	struct console *con;
> >> +	unsigned long flags;
> >>  	size_t len = 0;
> >>  
> >>  	/* only dump kmsg when no console is available */
> >> @@ -25,11 +28,16 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
> >>  	if (con)
> >>  		return;
> >>  
> >> +	if (!spin_trylock_irqsave(&lock, flags))
> >> +		return;
> >> +
> >>  	printf("kmsg_dump:\n");
> >>  	while (kmsg_dump_get_line(iter, true, line, sizeof(line), &len)) {
> >>  		line[len] = '\0';
> >>  		printf("%s", line);
> >>  	}
> >> +
> >> +	spin_unlock_irqrestore(&lock, flags);
> >
> > What exactly is synchronized here, please?
> > Access to @line buffer or @iter or both?
> 
> @line is being synchronized. @iter does not require synchronization.
> 
> > It looks to me that the access to @line buffer was not synchronized
> > before. kmsg_dump_get_line() used a lock internally but
> > it was not synchronized with the later printf("%s", line);
> 
> The line was previously synchronized for the kmsg_dump_get_line()
> call. But yes, it was not synchronized after the call, which is a bug if
> the dump is triggered on multiple CPUs simultaneously. The commit
> message should also mention that it is handling that bug.

Yes, please update the commit message.

In fact, I think that mentioning logbuf_lock might is a bit
misleading. This patch fixes an old bug. It was found when
auditing code that is using the kmsg_dump_get_line() and
might be affected the logbuf_lock removal.

Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Finally thanks Richard for the input about SMP on UM arch.

Best Regards,
Petr