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

[John Ogness]

Hi Richard,

On 2021-02-01, Richard Weinberger <richard@xxxxxx> 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@xxxxxxxxxxxxx>
>>>> ---
>>>>  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