The function cppc_freq_invariance_init() may failed to create
kworker_fie, make it more robust by checking the return value to prevent
an invalid pointer dereference in kthread_destroy_worker(), which called
from cppc_freq_invariance_exit().
Signed-off-by: Liao Chang <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 022e3555407c..4432398c8592 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -220,7 +220,7 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
}
}
-static void __init cppc_freq_invariance_init(void)
+static int __init cppc_freq_invariance_init(void)
{
struct sched_attr attr = {
.size = sizeof(struct sched_attr),
@@ -246,19 +246,23 @@ static void __init cppc_freq_invariance_init(void)
}
if (fie_disabled)
- return;
+ return 0;
kworker_fie = kthread_create_worker(0, "cppc_fie");
- if (IS_ERR(kworker_fie))
- return;
+ if (IS_ERR(kworker_fie)) {
+ ret = PTR_ERR(kworker_fie);
+ kworker_fie = NULL;
+ return ret;
+ }
ret = sched_setattr_nocheck(kworker_fie->task, &attr);
if (ret) {
pr_warn("%s: failed to set SCHED_DEADLINE: %d\n", __func__,
ret);
kthread_destroy_worker(kworker_fie);
- return;
+ kworker_fie = NULL;
}
+ return ret;
}
static void cppc_freq_invariance_exit(void)
@@ -279,8 +283,9 @@ static inline void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
{
}
-static inline void cppc_freq_invariance_init(void)
+static inline int cppc_freq_invariance_init(void)
{
+ return 0;
}
static inline void cppc_freq_invariance_exit(void)
@@ -969,7 +974,9 @@ static int __init cppc_cpufreq_init(void)
return -ENODEV;
cppc_check_hisi_workaround();
- cppc_freq_invariance_init();
+ ret = cppc_freq_invariance_init();
+ if (ret < 0)
+ return ret;
populate_efficiency_class();
ret = cpufreq_register_driver(&cppc_cpufreq_driver);
--
2.34.1
Hi Viresh,
在 2023/8/16 11:46, Viresh Kumar 写道:
> On 16-08-23, 03:05, Liao Chang wrote:
>> The function cppc_freq_invariance_init() may failed to create
>> kworker_fie, make it more robust by checking the return value to prevent
>> an invalid pointer dereference in kthread_destroy_worker(), which called
>> from cppc_freq_invariance_exit().
>>
>> Signed-off-by: Liao Chang <[email protected]>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> I think why it was designed this way was to make the driver work,
> without invariance support, in the worst case instead of just failing
> completely. The invariance thing is a good to have feature, but not
> really necessary and so failing probing the driver for that isn't
> worth it. We should print all error messages though.
>
Thanks for pointing that out. I think you are right that the kworker created
in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
updater has more priority than the driver, so the driver should still work even
without kworker_fie supports.
If that is the case, i think the best thing to do is checking the error pointer
and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
this is because at that point, it is really necessary to ensure the kworker_fie has
been initialized as expected, otherwise it will raise a NULL pointer exception.
I hope this makes sense, thanks.
--
BR
Liao, Chang
在 2023/8/16 16:17, Viresh Kumar 写道:
> On 16-08-23, 15:27, Liao, Chang wrote:
>> Hi Viresh,
>>
>> 在 2023/8/16 11:46, Viresh Kumar 写道:
>>> On 16-08-23, 03:05, Liao Chang wrote:
>>>> The function cppc_freq_invariance_init() may failed to create
>>>> kworker_fie, make it more robust by checking the return value to prevent
>>>> an invalid pointer dereference in kthread_destroy_worker(), which called
>>>> from cppc_freq_invariance_exit().
>>>>
>>>> Signed-off-by: Liao Chang <[email protected]>
>>>> ---
>>>> drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
>>>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> I think why it was designed this way was to make the driver work,
>>> without invariance support, in the worst case instead of just failing
>>> completely. The invariance thing is a good to have feature, but not
>>> really necessary and so failing probing the driver for that isn't
>>> worth it. We should print all error messages though.
>>>
>> Thanks for pointing that out. I think you are right that the kworker created
>> in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
>> updater has more priority than the driver, so the driver should still work even
>> without kworker_fie supports.
>>
>> If that is the case, i think the best thing to do is checking the error pointer
>> and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
>> this is because at that point, it is really necessary to ensure the kworker_fie has
>> been initialized as expected, otherwise it will raise a NULL pointer exception.
>
> Or just set fie_disabled to true ?
Yes, I agree.
>
>> I hope this makes sense, thanks.
>
> It does.
>
--
BR
Liao, Chang
On 16-08-23, 15:27, Liao, Chang wrote:
> Hi Viresh,
>
> 在 2023/8/16 11:46, Viresh Kumar 写道:
> > On 16-08-23, 03:05, Liao Chang wrote:
> >> The function cppc_freq_invariance_init() may failed to create
> >> kworker_fie, make it more robust by checking the return value to prevent
> >> an invalid pointer dereference in kthread_destroy_worker(), which called
> >> from cppc_freq_invariance_exit().
> >>
> >> Signed-off-by: Liao Chang <[email protected]>
> >> ---
> >> drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
> >> 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > I think why it was designed this way was to make the driver work,
> > without invariance support, in the worst case instead of just failing
> > completely. The invariance thing is a good to have feature, but not
> > really necessary and so failing probing the driver for that isn't
> > worth it. We should print all error messages though.
> >
> Thanks for pointing that out. I think you are right that the kworker created
> in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
> updater has more priority than the driver, so the driver should still work even
> without kworker_fie supports.
>
> If that is the case, i think the best thing to do is checking the error pointer
> and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
> this is because at that point, it is really necessary to ensure the kworker_fie has
> been initialized as expected, otherwise it will raise a NULL pointer exception.
Or just set fie_disabled to true ?
> I hope this makes sense, thanks.
It does.
--
viresh