Messages in this thread | | | Date | Mon, 1 Feb 2021 15:15:00 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk-rework 09/12] um: synchronize kmsg_dumper |
| |
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
| |