Messages in this thread | | | Date | Fri, 10 Mar 2023 12:11:41 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk_put |
| |
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
| |