Re: [PATCH v2] scsi: ufs: core: Fix data race in CPU latency PM QoS request handling

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 9/2/2025 8:39 PM, Peter Wang (王信友) wrote:
> On Tue, 2025-09-02 at 15:48 +0800, Zhongqiu Han wrote:
>> 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> 
>> 
>> The cpu_latency_qos_add/remove/update_request interfaces lack
>> internal
>> synchronization by design, requiring the caller to ensure thread
>> safety.
>> The current implementation relies on the `pm_qos_enabled` flag, which
>> is
>> insufficient to prevent concurrent access and cannot serve as a
>> proper
>> synchronization mechanism. This has led to data races and list
>> corruption
>> issues.
>> 
>> A typical race condition call trace is:
>> 
>> [Thread A]
>> ufshcd_pm_qos_exit()
>>   --> cpu_latency_qos_remove_request()
>>     --> cpu_latency_qos_apply();
>>       --> pm_qos_update_target()
>>         --> plist_del              <--(1) delete plist node
>>     --> memset(req, 0, sizeof(*req));
>>   --> hba->pm_qos_enabled = false;
>> 
>> [Thread B]
>> ufshcd_devfreq_target
>>   --> ufshcd_devfreq_scale
>>     --> ufshcd_scale_clks
>>       --> ufshcd_pm_qos_update     <--(2) pm_qos_enabled is true
>>         --> cpu_latency_qos_update_request
>>           --> pm_qos_update_target
>>             --> plist_del          <--(3) plist node use-after-free
>> 
>> This patch introduces a dedicated mutex to serialize PM QoS
>> operations,
>> preventing data races and ensuring safe access to PM QoS resources.
>> Additionally, READ_ONCE is used in the sysfs interface to ensure
>> atomic
>> read access to pm_qos_enabled flag.
> 
> 
> Hi Zhongqiu,
> 
> Introducing an additional mutex lock would impact the efficiency of
> devfreq.
> Wouldn’t it be better to simply adjust the sequence to avoid race
> conditions?
> For instance,
> ufshcd_pm_qos_exit(hba);
> ufshcd_exit_clk_scaling(hba);
> could be changed to
> ufshcd_exit_clk_scaling(hba);
> ufshcd_pm_qos_exit(hba);
> This ensures that clock scaling is stopped before pm_qos is removed.
> 
> Thanks.
> Peter
> 
> 
> 
> ************* MEDIATEK Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!
> 

Hi Peter,
Thank you for your review and suggestion~

1.While adjusting the sequence of operations may help avoid race
conditions, it is not a reliable solution and increases the risk of
future maintenance issues.

2.There are additional race paths beyond the one discussed. For example,
user-triggered sysfs interactions pm_qos_enable_store can occur at
unpredictable times, potentially leading to concurrent access to PM QoS
resources.

3.The critical section protected by the mutex is relatively small, so
the performance impact is expected to be minimal.

Thanks again for your feedback!


-- 
Thx and BRs,
Zhongqiu Han