Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR

From: Cezary Rojewski
Date: Sun Feb 23 2020 - 11:00:03 EST

On 2020-02-21 16:40, Pierre-Louis Bossart wrote:
On 2/21/20 8:41 AM, Joe Perches wrote:
On Fri, 2020-02-21 at 18:11 +0800, Xu Wang wrote:
PTR_ERR should access the value just tested by IS_ERR.
In skl_clk_dev_probe(),it is inconsistent.

Please include all maintainers of given driver when submitting the patch, thank you.

diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -384,7 +384,7 @@ static int skl_clk_dev_probe(struct platform_device *pdev)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clks[i], clk_pdata, i);
ÂÂÂÂÂÂÂÂÂ if (IS_ERR(data->clk[data->avail_clk_cnt])) {
-ÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
+ÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->clk[data->avail_clk_cnt]);


This is not inconsistent and you are removing the ++
which is a post increment. Likely that is necessary.

You could write the access and the increment as two
separate statements if it confuses you.

Well to be fair the code is far from clear.

Thanks for notifying, Pierre.

Although NAK is upheld here. Proposed change is likely to introduce regression.

the post-increment is likely needed because of the error handling in unregister_src_clk 1
ÂÂÂÂÂÂÂ data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clks[i], clk_pdata, i);

ÂÂÂÂÂÂÂ if (IS_ERR(data->clk[data->avail_clk_cnt])) {
ÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
ÂÂÂÂÂÂÂÂÂÂÂ goto err_unreg_skl_clk;

ÂÂÂÂplatform_set_drvdata(pdev, data);

ÂÂÂÂreturn 0;


static void unregister_src_clk(struct skl_clk_data *dclk)
ÂÂÂÂwhile (dclk->avail_clk_cnt--)
ÂÂÂÂÂÂÂ clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup);

So the post-increment is cancelled in the while().

That said, the avail_clk_cnt field is never initialized or incremented in normal usages so the code looks quite suspicious indeed.

As basically entire old Skylake code, so no surprises here : )
struct skl_clk_data::avail_clk_cnt field is initialized with 0 via devm_kzalloc in skl_clk_dev_probe().

gitk tells me this patch is likely the culprit:

6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev')

-ÂÂÂÂÂÂÂ data->clk[i] = register_skl_clk(dev, &clks[i], clk_pdata, i);
-ÂÂÂÂÂÂÂ if (IS_ERR(data->clk[i])) {
-ÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->clk[i]);
+ÂÂÂÂÂÂÂ data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clks[i], clk_pdata, i);
+ÂÂÂÂÂÂÂ if (IS_ERR(data->clk[data->avail_clk_cnt])) {
+ÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
ÂÂÂÂÂÂÂÂÂÂÂÂ goto err_unreg_skl_clk;
-ÂÂÂÂÂÂÂ data->avail_clk_cnt++;

That last removal is probably wrong. Cezary and Amadeusz, you may want to look at this?

Indeed, code looks wrong. Idk what are we even dropping in unregister_src_clk() if register_skl_clk() fails and avail_clk_cnt gets incremented anyway.

In general usage of while(ptr->counter--) (example of which is present in unregister_src_clk()) is prone to errors. Decrementation happens regardless of while's check outcome and caller may receive back handle in invalid state.

Amadeo, your thoughts?