linux-kernel - Re: [PATCH printk-rework 09/12] um: synchronize kmsg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87v9bbzga2.fsf@jogness.linutronix.de>
Date:   Mon, 01 Feb 2021 21:31:57 +0106
From:   John Ogness <john.ogness@...utronix.de>
To:     Richard Weinberger <richard@....at>
Cc:     Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        tglx <tglx@...utronix.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Thomas Meyer <thomas@...3r.de>
Subject: Re: [PATCH printk-rework 09/12] um: synchronize kmsg_dumper

Hi Richard,

On 2021-02-01, Richard Weinberger <richard@....at> 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@...utronix.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.
>> 
>>> 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.
>
> Well, uml has no SMP support, so no parallel dumps. :-)

When I grep through arch/um, I see many uses of spinlocks. This would
imply that uml at least has some sort of preemption model where they are
needed. Dumps can trigger from any context and from multiple paths.

If you are sure that this is no concern, then I will drop this patch
from my series.

John Ogness