2023-08-16 12:36:03

by Liao, Chang

Subject: [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie

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 <[email protected]>
---
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

2023-08-17 11:11:09

by Liao, Chang

Subject: Re: [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie

在 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 <[email protected]>
>> ---
>> 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

2023-08-19 20:00:18

by Viresh Kumar

Subject: Re: [PATCH v2] cpufreq: cppc: Set fie_disabled to FIE_DISABLED if fails to create kworker_fie

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 <[email protected]>
> ---
> 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