On 2021-06-24, Petr Mladek <pmla...@suse.com> 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

Reply via email to