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

[Petr Mladek <pmladek@suse.com>]

On Tue 2021-01-26 22:21:48, John Ogness 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?

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 only remaining reason might be the access to @iter. But it is on stack
in kmsg_dump(). Nobody else could have access to it.

IMHO, this patch is not needed.

Do I miss something, please?

Best Regards,
Petr