On 2021-06-24, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1852,7 +1839,7 @@ static int console_trylock_spinning(void)
>> if (console_trylock())
>> return 1;
>>
>> - printk_safe_enter_irqsave(flags);
>> + local_irq_save(flags);
>>
>> raw_spin_lock(&console_owner_lock);
>
> This spin_lock is in the printk() path. We must make sure that
> it does not cause deadlock.
>
> printk_safe_enter_irqsave(flags) prevented the recursion because
> it deferred the console handling.
>
> One danger might be a lockdep report triggered by
> raw_spin_lock(&console_owner_lock) itself. But it should be safe.
> lockdep is checked before the lock is actually taken
> and lockdep should disable itself before printing anything.
>
> Another danger might be any printk() called under the lock.
> The code just compares and assigns values to some variables
> (static, on stack) so we should be on the safe side.
>
> Well, I would feel more comfortable if we add printk_safe_enter_irqsave()
> back around the sections guarded by this lock. It can be done
> in a separate patch. The code looks safe at the moment.
You are correct. printk_safe should also be wrapping @console_owner_lock
locking.
>> @@ -2716,19 +2700,22 @@ void console_unlock(void)
>> * were to occur on another CPU, it may wait for this one to
>> * finish. This task can not be preempted if there is a
>> * waiter waiting to take over.
>> + *
>> + * Interrupts are disabled because the hand over to a waiter
>> + * must not be interrupted until the hand over is completed
>> + * (@console_waiter is cleared).
>> */
>> + local_irq_save(flags);
>> console_lock_spinning_enable();
>
> Same here. console_lock_spinning_enable() takes console_owner_lock.
> I would feel more comfortable if we added printk_safe_enter_irqsave(flags)
> inside console_lock_spinning_enable() around the locked code. The code
> is safe at the moment but...
Agreed.
>> stop_critical_timings(); /* don't trace print latency */
>> call_console_drivers(ext_text, ext_len, text, len);
>> start_critical_timings();
>>
>> - if (console_lock_spinning_disable_and_check()) {
>> - printk_safe_exit_irqrestore(flags);
>> + handover = console_lock_spinning_disable_and_check();
>
> Same here. Also console_lock_spinning_disable_and_check() takes
> console_owner_lock. It looks safe at the moment but...
Agreed.
>> --- a/kernel/printk/printk_safe.c
>> +++ b/kernel/printk/printk_safe.c
>> @@ -369,7 +70,10 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>> * Use the main logbuf even in NMI. But avoid calling console
>> * drivers that might have their own locks.
>> */
>> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
>> + if (this_cpu_read(printk_context) &
>> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
>> + PRINTK_NMI_CONTEXT_MASK |
>> + PRINTK_SAFE_CONTEXT_MASK)) {
>> unsigned long flags;
>> int len;
>>
>
> There is the following code right below:
>
> printk_safe_enter_irqsave(flags);
> len = vprintk_store(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
> printk_safe_exit_irqrestore(flags);
> defer_console_output();
> return len;
>
> printk_safe_enter_irqsave(flags) is not needed here. Any nested
> printk() ends here as well.
Ah, I missed that one. Good eye!
> Against this can be done in a separate patch. Well, the commit message
> mentions that the printk_safe context is removed everywhere except
> for the code manipulating console lock. But is it just a detail.
I would prefer a v4 with these fixes:
- wrap @console_owner_lock with printk_safe usage
- remove unnecessary printk_safe usage from printk_safe.c
- update commit message to say that safe context tracking is left in
place for both the console and console_owner locks
John Ogness
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
