linux-kernel - Re: [PATCH] cpufreq: Fix the race condition while updating the transition

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

Message-ID: <bcac1b3c-a2f7-7f18-4c2f-5cf27f4e89b5@huawei.com>
Date:   Mon, 28 Aug 2023 16:29:02 +0800
From:   "Liao, Chang" <liaochang1@...wei.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
CC:     <rafael@...nel.org>, <srivatsa.bhat@...ux.vnet.ibm.com>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpufreq: Fix the race condition while updating the
 transition_task of policy

Hi Viresh.

在 2023/8/28 15:23, Viresh Kumar 写道:
> On 26-08-23, 09:58, Liao Chang wrote:
>> The field 'transition_task' of policy structure is used to track the
>> task which is performing the frequency transition. Using this field to
>> print a warning once detect a case where the same task is calling
>> _begin() again before completing the preivous frequency transition via
>> the _end().
>>
>> However, there is a potential race condition in _end() and _begin() APIs
>> while updating the field 'transition_task' of policy, the scenario is
>> depicted below:
>>
>>              Task A                            Task B
>>
>>         /* 1st freq transition */
>>         Invoke _begin() {
>>                 ...
>>                 ...
>>         }
>>                                         /* 2nd freq transition */
>>                                         Invoke _begin() {
>>                                                 ... //waiting for A to
>>                                                 ... //clear
>>                                                 ... //transition_ongoing
>>                                                 ... //in _end() for
>>                                                 ... //the 1st transition
>>                                                         |
>>         Change the frequency                            |
>>                                                         |
>>         Invoke _end() {                                 |
>>                 ...                                     |
>>                 ...                                     |
>>                 transition_ongoing = false;             V
>>                                                 transition_ongoing = true;
>>                                                 transition_task = current;
> 
> Task B here won't move ahead until "wake_up(&policy->transition_wait)"
> is called, isn't it ?

Task B does not necessarily go to sleep when it calls wait_event(), it depends on
the condition to wait for evaluate false or not. So there is a small race window
where Task A already set 'transition_ongoing' to false and Task B can cross wait_event()
immediately.

wait_event:
do {
	might_sleep();
	if (condition) // !transition_ongoing
		break;
	__wait_event();
};

I hope I do not miss something important in the code above.

> 
> Also I think the CPU is free to change the order of the two
> instructions and so this commit won't make a difference. Also I don't

Yes, if the CPU uses weak memroy model, it is possible for the instructions to be reordered.
therefore, it is a good idea to insert an smb() between these two lines if there is race here.

Thanks.

> feel there is a race here as wake_up() hasn't happened.
> 
>>                 transition_task = NULL;
>>                 ... //A overwrites the task
>>                 ... //performing the transition
>>                 ... //result in error warning.
>>         }
>>
>> To fix this race condition, the order of the updates to the
>> 'transition_task' and 'transition_ongoing' fields has been changed, the
>> 'transition_task' field is now cleared before the 'transition_ongoing'
>> field, which ensure that only one task can update the 'transition_task'
>> field at a time.
>>
>> Fixes: ca654dc3a93d ("cpufreq: Catch double invocations of cpufreq_freq_transition_begin/end")
>> Signed-off-by: Liao Chang <liaochang1@...wei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index a757f90aa9d6..f8eb6dde57f2 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -455,8 +455,8 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
>>  			    policy->cur,
>>  			    policy->cpuinfo.max_freq);
>>  
>> -	policy->transition_ongoing = false;
>>  	policy->transition_task = NULL;
>> +	policy->transition_ongoing = false;
>>  
>>  	wake_up(&policy->transition_wait);
>>  }
>> -- 
>> 2.34.1
> 

-- 
BR
Liao, Chang