On 03/07/2025 5:01 pm, Roger Pau Monné wrote:
> On Wed, Jul 02, 2025 at 03:41:16PM +0100, Andrew Cooper wrote:
>> cpuidle_wakeup_mwait() is a TOCTOU race. The cpumask_and() sampling
>> cpuidle_mwait_flags can take a arbitrary period of time, and there's no
>> guarantee that the target CPUs are still in MWAIT when writing into
>> mwait_wakeup(cpu).
>>
>> The consequence of the race is that we'll fail to IPI targets. Also, there's
>> no guarantee that mwait_idle_with_hints() will raise a TIMER_SOFTIRQ on it's
>> way out.
>>
>> The fundamental bug is that the "in_mwait" variable needs to be in the
>> monitored line in order to do this in a race-free way.
> I assume in_mwait being in the same monitored line is required so that
> you can do an atomic exchange and ensure the remote CPU was either in
> the monitor state, or just getting out of it but before processing
> softirqs when the softirq is set?
Yes
> That means that a CPU getting out of mwait would also need to do an
> atomic exchange to clear in_mwait and fetch the pending softirqs?
Not quite. It's probably better to see patch 4, than discuss it here.
>
>> Arranging for this while keeping the old implementation is prohibitive, so
>> strip it out in order to implement the new one cleanly. In the interim, this
>> causes IPIs to be used unconditionally which is safe albeit suboptimal.
>>
>> Fixes: 3d521e933e1b ("cpuidle: mwait on softirq_pending & remove wakeup
>> ipis")
>> Fixes: 1adb34ea846d ("CPUIDLE: re-implement mwait wakeup process")
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks
~Andrew
