linux-kernel - Re: [PATCH v2] cpu_pm: Make notifier chain use a raw spinlock

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] [day] [month] [year] [list]

Date:   Wed, 11 Aug 2021 18:59:43 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Ingo Molnar <mingo@...nel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v2] cpu_pm: Make notifier chain use a raw spinlock

On 11/08/21 15:52, Sebastian Andrzej Siewior wrote:
> On 2021-08-11 14:14:05 [+0100], Valentin Schneider wrote:
>> Booting a recent PREEMPT_RT kernel (v5.14-rc5-rt8 with the previous version
>> of this fix reverted) on my arm4 Juno leads to the idle task blocking on a
>> sleeping spinlock down some notifier path:
>> 
>> [    5.163034] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:35
[...]
>> [    5.163294] __secondary_switched (arch/arm64/kernel/head.S:661)
>
> I would shrink that part above. The important part is that the CPU-idle
> code runs with disabled interrupts. Then cpu_pm_notify_robust() invokes
> the notifier which requires to acquire the spinlock_t. On PREEMPT_RT the
> spinlock_t becomes a sleeping spinlock and must not be acquired with
> disabled interrupts.

Noted, I'll pluck the warning out.

>> +/*
>> + * atomic_notifiers use a regular spinlock, but notifications for this chain
>> + * will be issued by the idle task which cannot block.
>
> Maybe + a few details and make it more explicit
>
>  * atomic_notifiers use a spinlock_t, but notifications for this chain
>  * will be issued by the idle task with disabled interrupts which cannot
>  * block on PREEMPT_RT.
>
> ?
>

More generally I'd say the idle task is never preemptible (as in
preempt_count > 0 at all times), so any notification issued by the idle
task itself cannot block. The fact those are also issued in an IRQ-off
region just further cements that.

> …
>> @@ -33,10 +45,13 @@ static int cpu_pm_notify(enum cpu_pm_event event)
>>  
>>  static int cpu_pm_notify_robust(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
>>  {
>> +	unsigned long flags;
>>  	int ret;
>>  
>>  	rcu_irq_enter_irqson();
>> -	ret = atomic_notifier_call_chain_robust(&cpu_pm_notifier_chain, event_up, event_down, NULL);
>
> could we get rid of atomic_notifier_call_chain_robust() now that we have
> zero users?
>

No objections from my end, I'll add that in v3 and see if anyone complains.

>> +	raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
>> +	ret = raw_notifier_call_chain_robust(&cpu_pm_notifier.chain, event_up, event_down, NULL);
>> +	raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);
>>  	rcu_irq_exit_irqson();
>>  
>>  	return notifier_to_errno(ret);
>
> Sebastian