linux-kernel - Re: [PATCH] ata: sata_dwc

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]

Date:	Fri, 18 Feb 2011 20:27:42 +0300
From:	Vasiliy Kulikov <segooon@...il.com>
To:	Sergei Shtylyov <sshtylyov@...sta.com>
Cc:	Jeff Garzik <jgarzik@...ox.com>, kernel-janitors@...r.kernel.org,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Rupjyoti Sarmah <rsarmah@...liedmicro.com>,
	Prodyut Hazarika <phazarika@...liedmicro.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: sata_dwc_460ex: fix error path

Hi Sergei,

On Wed, Feb 16, 2011 at 21:10 +0300, Sergei Shtylyov wrote:
> >@@ -727,11 +727,15 @@ static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
> > static void dma_dwc_exit(struct sata_dwc_device *hsdev)
> > {
> > 	dev_dbg(host_pvt.dwc_dev, "%s:\n", __func__);
> >-	if (host_pvt.sata_dma_regs)
> >+	if (host_pvt.sata_dma_regs) {
> > 		iounmap(host_pvt.sata_dma_regs);
> >+		host_pvt.sata_dma_regs = NULL;
> >+	}
> >-	if (hsdev->irq_dma)
> >+	if (hsdev->irq_dma) {
> > 		free_irq(hsdev->irq_dma, hsdev);
> >+		hsdev->irq_dma = 0;
> >+	}
> > }
> 
>    Are those changes really necessary, i.e. is dma_dwc_exit() called
> more than once?

Because this function may be called once again before
host_pvt.sata_dma_regs gains new value in sata_dwc_probe().  This would
lead to double iounmap() and double free_irq().

> >@@ -1606,7 +1610,7 @@ static int sata_dwc_probe(struct platform_device *ofdev,
> > 	if (hsdev == NULL) {
> > 		dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> > 		err = -ENOMEM;
> >-		goto error_out;
> >+		goto error;
> 
>    Why not just:
> 
> 		return err;

If some resource allocation is added before this line in future then
"return err" should be changed.  With goto the single cleanup is just
added to the end of the function.


Thanks,

-- 
Vasiliy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/