* [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie
@ 2023-08-16 9:49 Liao Chang
2023-08-16 10:37 ` Viresh Kumar
0 siblings, 1 reply; 3+ messages in thread
From: Liao Chang @ 2023-08-16 9:49 UTC (permalink / raw)
To: rafael, viresh.kumar; +Cc: linux-pm, linux-kernel
The function cppc_freq_invariance_init() may failed to create
kworker_fie, make it more robust by setting fie_disabled to FIE_DISBALED
to prevent an invalid pointer dereference in kthread_destroy_worker(),
which called from cppc_freq_invariance_exit().
Link: https://lore.kernel.org/all/20230816034630.a4hvsj373q6aslk3@vireshk-i7/
Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
drivers/cpufreq/cppc_cpufreq.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 022e3555407c..bff4cde06083 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -220,6 +220,15 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
}
}
+static void cppc_freq_invariance_exit(void)
+{
+ if (fie_disabled)
+ return;
+
+ kthread_destroy_worker(kworker_fie);
+ kworker_fie = NULL;
+}
+
static void __init cppc_freq_invariance_init(void)
{
struct sched_attr attr = {
@@ -249,27 +258,22 @@ static void __init cppc_freq_invariance_init(void)
return;
kworker_fie = kthread_create_worker(0, "cppc_fie");
- if (IS_ERR(kworker_fie))
+ if (IS_ERR(kworker_fie)) {
+ pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
+ PTR_ERR(kworker_fie));
+ fie_disabled = FIE_DISABLED;
return;
+ }
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;
+ cppc_freq_invariance_exit();
+ fie_disabled = FIE_DISABLED;
}
}
-static void cppc_freq_invariance_exit(void)
-{
- if (fie_disabled)
- return;
-
- kthread_destroy_worker(kworker_fie);
- kworker_fie = NULL;
-}
-
#else
static inline void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie
2023-08-16 9:49 [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie Liao Chang
@ 2023-08-16 10:37 ` Viresh Kumar
2023-08-17 2:33 ` Liao, Chang
0 siblings, 1 reply; 3+ messages in thread
From: Viresh Kumar @ 2023-08-16 10:37 UTC (permalink / raw)
To: Liao Chang; +Cc: rafael, linux-pm, linux-kernel
On 16-08-23, 09:49, Liao Chang wrote:
> The function cppc_freq_invariance_init() may failed to create
> kworker_fie, make it more robust by setting fie_disabled to FIE_DISBALED
> to prevent an invalid pointer dereference in kthread_destroy_worker(),
> which called from cppc_freq_invariance_exit().
>
> Link: https://lore.kernel.org/all/20230816034630.a4hvsj373q6aslk3@vireshk-i7/
>
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 022e3555407c..bff4cde06083 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -220,6 +220,15 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
> }
> }
>
> +static void cppc_freq_invariance_exit(void)
> +{
> + if (fie_disabled)
> + return;
> +
> + kthread_destroy_worker(kworker_fie);
> + kworker_fie = NULL;
> +}
> +
> static void __init cppc_freq_invariance_init(void)
> {
> struct sched_attr attr = {
> @@ -249,27 +258,22 @@ static void __init cppc_freq_invariance_init(void)
> return;
>
> kworker_fie = kthread_create_worker(0, "cppc_fie");
> - if (IS_ERR(kworker_fie))
> + if (IS_ERR(kworker_fie)) {
> + pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
> + PTR_ERR(kworker_fie));
> + fie_disabled = FIE_DISABLED;
> return;
> + }
>
> 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;
> + cppc_freq_invariance_exit();
I don't really like this change, there aren't a lot of things that we
need to do here on cleanup, but just kthread_destroy_worker(). Calling
it directly makes more sense I guess.
> + fie_disabled = FIE_DISABLED;
> }
> }
>
> -static void cppc_freq_invariance_exit(void)
> -{
> - if (fie_disabled)
> - return;
> -
> - kthread_destroy_worker(kworker_fie);
> - kworker_fie = NULL;
I don't see the point of this line. Probably it can be dropped.
> -}
> -
> #else
> static inline void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
> {
> --
> 2.34.1
--
viresh
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie
2023-08-16 10:37 ` Viresh Kumar
@ 2023-08-17 2:33 ` Liao, Chang
0 siblings, 0 replies; 3+ messages in thread
From: Liao, Chang @ 2023-08-17 2:33 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rafael, linux-pm, linux-kernel
在 2023/8/16 18:37, Viresh Kumar 写道:
> On 16-08-23, 09:49, Liao Chang wrote:
>> The function cppc_freq_invariance_init() may failed to create
>> kworker_fie, make it more robust by setting fie_disabled to FIE_DISBALED
>> to prevent an invalid pointer dereference in kthread_destroy_worker(),
>> which called from cppc_freq_invariance_exit().
>>
>> Link: https://lore.kernel.org/all/20230816034630.a4hvsj373q6aslk3@vireshk-i7/
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 28 ++++++++++++++++------------
>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 022e3555407c..bff4cde06083 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -220,6 +220,15 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
>> }
>> }
>>
>> +static void cppc_freq_invariance_exit(void)
>> +{
>> + if (fie_disabled)
>> + return;
>> +
>> + kthread_destroy_worker(kworker_fie);
>> + kworker_fie = NULL;
>> +}
>> +
>> static void __init cppc_freq_invariance_init(void)
>> {
>> struct sched_attr attr = {
>> @@ -249,27 +258,22 @@ static void __init cppc_freq_invariance_init(void)
>> return;
>>
>> kworker_fie = kthread_create_worker(0, "cppc_fie");
>> - if (IS_ERR(kworker_fie))
>> + if (IS_ERR(kworker_fie)) {
>> + pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
>> + PTR_ERR(kworker_fie));
>> + fie_disabled = FIE_DISABLED;
>> return;
>> + }
>>
>> 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;
>> + cppc_freq_invariance_exit();
>
> I don't really like this change, there aren't a lot of things that we
> need to do here on cleanup, but just kthread_destroy_worker(). Calling
> it directly makes more sense I guess.
OK，I will simplify code in next revisoin.
>
>> + fie_disabled = FIE_DISABLED;
>> }
>> }
>>
>> -static void cppc_freq_invariance_exit(void)
>> -{
>> - if (fie_disabled)
>> - return;
>> -
>> - kthread_destroy_worker(kworker_fie);
>
>> - kworker_fie = NULL;
>
> I don't see the point of this line. Probably it can be dropped.
I agree with you, when fie_disbaled is set to FIE_DISABLED and kthred_destroy_worker() is called,
the driver has no chance to access the invalid kworker_fie pointer.
Thanks.
>
>> -}
>> -
>> #else
>> static inline void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
>> {
>> --
>> 2.34.1
>
--
BR
Liao, Chang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-17 2:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-16 9:49 [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie Liao Chang
2023-08-16 10:37 ` Viresh Kumar
2023-08-17 2:33 ` Liao, Chang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).