On Mon 2021-02-01 17:57:14, John Ogness wrote: > (Added CC: Thomas Meyer, Richard Weinberger) > > On 2021-02-01, Petr Mladek <[email protected]> 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 <[email protected]> > >> --- > >> 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 <[email protected]> Finally thanks Richard for the input about SMP on UM arch. Best Regards, Petr
