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] [thread-next>] [day] [month] [year] [list]

Message-ID: <xxtrhbv5qm2crtvc5ejpgu5caadsmms3rfulmosjwq7lumrko3@5mlcpk24hymm>
Date: Sun, 12 Oct 2025 16:15:44 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Khalid Aziz <khalid@...nel.org>
Cc: Rakuram Eswaran <rakuram.e96@...il.com>, chenhuacai@...nel.org, 
	dan.carpenter@...aro.org, david.hunter.linux@...il.com, 
	linux-kernel-mentees@...ts.linux.dev, linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org, 
	lkp@...el.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()

On Fri, Oct 10, 2025 at 11:59:57AM -0600, Khalid Aziz wrote:
> On 10/10/25 3:59 AM, Uwe Kleine-König wrote:
> > Hello Rakuram,
> > 
> > On Thu, Oct 09, 2025 at 08:57:38PM +0530, Rakuram Eswaran wrote:
> > > Your suggestion makes perfect sense — since host is devm-managed,
> > > explicitly assigning its members to NULL has no effect.
> > > I’ll remove those two redundant lines in v2 as you suggested.
> > > 
> > > I had one small clarification regarding the remaining host->dma_chan_tx = NULL;
> > > in the TX DMA error path. Since that branch uses goto out,
> > > the cleanup section below may call dma_release_channel() on both RX
> > > and TX pointers. Setting TX to NULL there seems like a defensive step
> > > to avoid accidentally passing an ERR_PTR() to dma_release_channel()
> > > — is that understanding correct?
> > 
> > Ah right, so either keep host->dma_chan_tx = NULL or improve the error
> > handling like:
> > 
> > diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> > index 26d03352af63..e5068cc55fb2 100644
> > --- a/drivers/mmc/host/pxamci.c
> > +++ b/drivers/mmc/host/pxamci.c
> > @@ -715,7 +715,7 @@ static int pxamci_probe(struct platform_device *pdev)
> >   		dev_err(dev, "unable to request tx dma channel\n");
> >   		ret = PTR_ERR(host->dma_chan_tx);
> >   		host->dma_chan_tx = NULL;
> > -		goto out;
> > +		goto out_dma_tx;
> >   	}
> >   	if (host->pdata) {
> > @@ -765,10 +765,11 @@ static int pxamci_probe(struct platform_device *pdev)
> >   	return 0;
> >   out:
> > -	if (host->dma_chan_rx)
> > -		dma_release_channel(host->dma_chan_rx);
> >   	if (host->dma_chan_tx)
> >   		dma_release_channel(host->dma_chan_tx);
> > +out_dma_tx:
> > +	if (host->dma_chan_rx)
> > +		dma_release_channel(host->dma_chan_rx);
> >   	return ret;
> >   }
> 
> 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.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)