(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.
> IMHO, this patch is not needed.
I am not familiar enough with ARCH=um to know if dumps can be triggered
on multiple CPUs simultaneously. Perhaps ThomasM or Richard can chime in
here.
John Ogness
