linux-kernel - Re: [PATCH] mmc: pxamci: Fix passing NULL to PTR_ERR() in pxamci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20251012183804.15171-1-rakuram.e96@gmail.com>
Date: Mon, 13 Oct 2025 00:07:52 +0530
From: Rakuram Eswaran <rakuram.e96@...il.com>
To: u.kleine-koenig@...libre.com
Cc: chenhuacai@...nel.org,
	dan.carpenter@...aro.org,
	david.hunter.linux@...il.com,
	khalid@...nel.org,
	linux-kernel-mentees@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	linux-mmc@...r.kernel.org,
	lkp@...el.com,
	rakuram.e96@...il.com,
	skhan@...uxfoundation.org,
	ulf.hansson@...aro.org,
	zhoubinbin@...ngson.cn
Subject: Re: [PATCH] mmc: pxamci: Fix passing NULL to PTR_ERR() in pxamci_probe() 

> >
> > I do not see the need for this code change. "if (host->dma_chan_tx)" will
> > skip "dma_release_channel(host->dma_chan_tx)" since dma_chan_tx is already
> > NULL. This code change does not add anything.
>
> Yes, stand alone this change doesn't make sense, but if we want to drop
>
>         host->dma_chan_tx = NULL
>
> in the error path above, this change is needed. Maybe then even
>
>         if (host->dma_chan_rx)
>
> and
>
>         if (host->dma_chan_rx)
>
> can be dropped.

Hello Uwe, 

I had one quick follow-up before sending v2.

Regarding the devm_clk_get() error path —
you mentioned that setting host->clk = NULL; is redundant since host is 
devm-managed and the function returns immediately afterward.

> I am not sure that sounds right. Looking at the code for
> __devm_clk_get(), if devres_alloc() fails, it returns -ENOMEM. If any of
> the other steps after a successful devres_alloc() fail, code goes
> through possibly clk_put() if needed and then devres_free(). So the
> resources are already freed at this point before the return to
> pxamci_probe(). The only thing left to do is to set host->clk to NULL
> since it would be set to an error pointer at this point.

Khalid pointed out that when __devm_clk_get() fails after allocating a 
devres entry, the internal cleanup (clk_put() + devres_free()) ensures 
resources are released, but host->clk would still hold an ERR_PTR() 
value at that point.

His suggestion was that setting it to NULL might be a harmless defensive 
step to avoid any accidental later dereference.

For now, I have dropped the redundant NULL assignment from 
host->dma_chan_rx = NULL and directly returning the ERR_PTR instead of 
storing in a return variable. 

Below I have appended proposed changes for v2.

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 26d03352af63..eb46a4861dbe 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -653,8 +653,9 @@ static int pxamci_probe(struct platform_device *pdev)

 	host->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(host->clk)) {
+		ret = PTR_ERR(host->clk);
 		host->clk = NULL;
-		return PTR_ERR(host->clk);
+		return ret;
 	}

 	host->clkrate = clk_get_rate(host->clk);
@@ -705,7 +706,6 @@ static int pxamci_probe(struct platform_device *pdev)

 	host->dma_chan_rx = dma_request_chan(dev, "rx");
 	if (IS_ERR(host->dma_chan_rx)) {
-		host->dma_chan_rx = NULL;
 		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
 				     "unable to request rx dma channel\n");
 	}

Would you prefer that I:

1. Remove the host->clk = NULL; assignment for consistency (as you initially 
suggested), or

2. Keep it in v2 for defensive clarity, as Khalid reasoned?

I just wanted to confirm your preference before resending, to keep v2 aligned.

Thanks again for your time and detailed feedback!

Best regards,
Rakuram