diff mbox series

cpufreq: cppc: Add missing error pointer check

Message ID 20230816030549.849824-1-liaochang1@huawei.com
State New
Headers
Series cpufreq: cppc: Add missing error pointer check |

Commit Message

Liao Chang Aug. 16, 2023, 3:05 a.m. UTC 
  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 <liaochang1@huawei.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Liao Chang Aug. 16, 2023, 7:27 a.m. UTC | #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 <liaochang1@huawei.com>
>> ---
>>  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.
diff mbox series

Patch

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);