Re: [PATCH] cpufreq: cppc_cpufreq: prevent crash on reading freqdomain_cpus

[Chris Hyser <chris.hyser@oracle.com>]

On 8/22/22 9:19 AM, Chris Hyser wrote:
> 
> 
> On 8/22/22 1:39 AM, Viresh Kumar wrote:
>> On 19-08-22, 15:03, Chris Hyser wrote:
>>> From: chris hyser <chris.hyser@oracle.com>
>>>
>>> While running stress-ng --sysfs on an ARM system following a cpu offline,
>>> we encountered the following NULL pointer dereference in the cppc_cpufreq
>>> scaling driver:
>>>
>>> [ 1003.576816] Call trace:
>>> [ 1003.579255]  _find_next_bit+0x20/0xc8
>>> [ 1003.582909]  cpufreq_show_cpus+0x78/0xf4
>>> [ 1003.586830]  show_freqdomain_cpus+0x20/0x30 [cppc_cpufreq]
>>> [ 1003.592318]  show+0x4c/0x78
>>> [ 1003.595104]  sysfs_kf_seq_show+0x9
>>>
>>> This is the exact issue described in commit e25303676e18 ("cpufreq:
>>> acpi_cpufreq: prevent crash on reading freqdomain_cpus") with the fix
>>> described there also solving this issue. I tracked the root cause to the
>>> following: a scaling driver which provides a struct freq_attr sysfs
>>> attributes array passed via struct cpufreq_driver during driver
>>> registration, has .init() and .exit() functions and does _not_ provide
>>> .online()/.offline() routines. cpufreq core creates the attributes, but
>>> does not remove them even though .exit() frees the underlying memory. The
>>> core functions and most drivers have corresponding NULL data pointer
>>> checks.
>>>
>>> Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
>>> ---
>>>   drivers/cpufreq/cppc_cpufreq.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index 24eaf0e..4210353 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -876,6 +876,9 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>>>   {
>>>       struct cppc_cpudata *cpu_data = policy->driver_data;
>>> +    if (unlikely(!cpu_data))
>>> +        return -ENODEV;
>>> +
>>>       return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
>>>   }
>>>   cpufreq_freq_attr_ro(freqdomain_cpus);
>>
>> What kernel version are you testing this on ?
> 
> 5.19
> 
>> We merged a patch sometime back:
>>
>> commit d4627a287e25 ("cpufreq: Abort show()/store() for half-initialized policies")
>>
>> which I believe should have fixed this issue. I will be surprise if it
>> doesn't.
> 
> This patch is present and apparently does not solve the problem.

On digging into it, that patch says it is about catching partially initialized policies and indicates that by clearing 
the policy->cpus mask. However, on a normal online/offline a completely initialized policy survives (ptr in the percpu 
space) and that mask is not cleared and the driver show() funcs get called.

-chrish