| Message ID | 20230310051750.4745-3-jia-wei.chang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance mediatek-cpufreq robustness | expand |
On Fri, Mar 10, 2023 at 01:17:49PM +0800, jia-wei.chang wrote: > From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com> > > Any kind of failure in mtk_cpu_dvfs_info_init() will lead to calling > regulator_put() or clk_put() and the KP will occur since the regulator/clk > handlers are used after released in mtk_cpu_dvfs_info_release(). > This patch is harmless but it's not required. If the mtk_cpu_dvfs_info_init() function is not able to complete successfully then it cleans up all the partial allocations. If it is able to allocate everything successfully, then the caller, which is mtk_cpufreq_probe(), adds it to the &dvfs_info_list. If the probe() function is not able to complete successfully it releases everything in the &dvfs_info_list. In this way, mtk_cpu_dvfs_info_init() never frees anything which has not been allocated and it never frees anything which has already been freed. All the IS_ERR() checks in mtk_cpu_dvfs_info_release() can be removed. In reviewing this code, I can't help but notice that mtk_cpu_dvfs_info_init() is buggy. Please read my blog on error handling: https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/ drivers/cpufreq/mediatek-cpufreq.c 411 info->cpu_clk = clk_get(cpu_dev, "cpu"); 412 if (IS_ERR(info->cpu_clk)) { 413 ret = PTR_ERR(info->cpu_clk); 414 return dev_err_probe(cpu_dev, ret, ^^^^^^^^^^^^^^^^^^^^ Here we have a direct return which means there is nothing to free. Okay. 415 "cpu%d: failed to get cpu clk\n", cpu); 416 } 417 418 info->inter_clk = clk_get(cpu_dev, "intermediate"); 419 if (IS_ERR(info->inter_clk)) { 420 ret = PTR_ERR(info->inter_clk); 421 dev_err_probe(cpu_dev, ret, 422 "cpu%d: failed to get intermediate clk\n", cpu); 423 goto out_free_resources; ^^^^^^^^^^^^^^^^^^^^^^^ The last thing we successfully allocated was ->cpu_clk so ideally the goto would be something like "goto put_cpu_clk;". Let's assume this clk_get() fails. 424 } 425 426 info->proc_reg = regulator_get_optional(cpu_dev, "proc"); ^^^^^^^^^^^^^^ Note that this is where ->proc_reg is allocated. Prior to that point it is NULL because it's allocated with kzalloc(). 427 if (IS_ERR(info->proc_reg)) { 428 ret = PTR_ERR(info->proc_reg); 429 dev_err_probe(cpu_dev, ret, 430 "cpu%d: failed to get proc regulator\n", cpu); 431 goto out_free_resources; 432 } [ snip ] 536 out_free_resources: 537 if (regulator_is_enabled(info->proc_reg)) ^^^^^^^^^^^^^^ Oops! This is a NULL dereference because ->proc_reg wasn't allocated yet. 538 regulator_disable(info->proc_reg); 539 if (info->sram_reg && regulator_is_enabled(info->sram_reg)) 540 regulator_disable(info->sram_reg); 541 542 if (!IS_ERR(info->proc_reg)) 543 regulator_put(info->proc_reg); 544 if (!IS_ERR(info->sram_reg)) 545 regulator_put(info->sram_reg); 546 if (!IS_ERR(info->cpu_clk)) 547 clk_put(info->cpu_clk); 548 if (!IS_ERR(info->inter_clk)) 549 clk_put(info->inter_clk); 550 551 return ret; 552 } It would be safer and more readable to have: err_put_inter_clk: clk_put(info->inter_clk); err_put_cpu_clk: clk_put(info->cpu_clk); etc. regards, dan carpenter
Il 10/03/23 06:17, jia-wei.chang ha scritto: > From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com> > > Any kind of failure in mtk_cpu_dvfs_info_init() will lead to calling > regulator_put() or clk_put() and the KP will occur since the regulator/clk > handlers are used after released in mtk_cpu_dvfs_info_release(). > > To prevent the usage after regulator_put()/clk_put(), the regulator/clk > handlers are reassigned to NULL value for validation check afterwards. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Fixes: 4b9ceb757bbb ("cpufreq: mediatek: Enable clocks and regulators") > Reported-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Link: https://lore.kernel.org/linux-arm-kernel/20220921071913.p7kwsjnnuad2jgvk@vireshk-i7/T/ Actually, it's not just Reported-by, but also please add: Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Anyway, apart from that: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 01d949707c37..cb8b76f9c2c3 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -539,35 +539,47 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) if (info->sram_reg && regulator_is_enabled(info->sram_reg)) regulator_disable(info->sram_reg); - if (!IS_ERR(info->proc_reg)) + if (!IS_ERR(info->proc_reg)) { regulator_put(info->proc_reg); - if (!IS_ERR(info->sram_reg)) + info->proc_reg = NULL; + } + if (!IS_ERR(info->sram_reg)) { regulator_put(info->sram_reg); - if (!IS_ERR(info->cpu_clk)) + info->sram_reg = NULL; + } + if (!IS_ERR(info->cpu_clk)) { clk_put(info->cpu_clk); - if (!IS_ERR(info->inter_clk)) + info->cpu_clk = NULL; + } + if (!IS_ERR(info->inter_clk)) { clk_put(info->inter_clk); + info->inter_clk = NULL; + } return ret; } static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) { - if (!IS_ERR(info->proc_reg)) { + if (!IS_ERR_OR_NULL(info->proc_reg)) { regulator_disable(info->proc_reg); regulator_put(info->proc_reg); + info->proc_reg = NULL; } - if (!IS_ERR(info->sram_reg)) { + if (!IS_ERR_OR_NULL(info->sram_reg)) { regulator_disable(info->sram_reg); regulator_put(info->sram_reg); + info->sram_reg = NULL; } - if (!IS_ERR(info->cpu_clk)) { + if (!IS_ERR_OR_NULL(info->cpu_clk)) { clk_disable_unprepare(info->cpu_clk); clk_put(info->cpu_clk); + info->cpu_clk = NULL; } - if (!IS_ERR(info->inter_clk)) { + if (!IS_ERR_OR_NULL(info->inter_clk)) { clk_disable_unprepare(info->inter_clk); clk_put(info->inter_clk); + info->inter_clk = NULL; } dev_pm_opp_of_cpumask_remove_table(&info->cpus);