[PATCH] staging, comedi, das16: das16_attach() needs to free allocated resources on failure

[PATCH] staging, comedi, das16: das16_attach() needs to free allocated resources on failure

[Jesper Juhl]

Hi,

In drivers/staging/comedi/drivers/das16.c::das16_attach() there are many 
potential failure scenarios. However, the driver neglects to free 
resources it has allocated if failures happen. This patch changes that so 
that all allocated resources are freed on error in proper order.

Unfortunately I have no hardware to test this patch with, so it is compile 
tested only.

Please review and consider for inclusion (and please CC me on replies).


Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 das16.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das16.c b/drivers/staging/comedi/drivers/das16.c
index 0af1b46..75ee2f3 100644
--- a/drivers/staging/comedi/drivers/das16.c
+++ b/drivers/staging/comedi/drivers/das16.c
@@ -1507,19 +1507,30 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	dma_chan = it->options[2];
 	if (dma_chan == 1 || dma_chan == 3) {
 		/*  allocate dma buffers */
-		int i;
-		for (i = 0; i < 2; i++) {
-			devpriv->dma_buffer[i] = pci_alloc_consistent(
-						NULL, DAS16_DMA_SIZE,
-						&devpriv->dma_buffer_addr[i]);
+		ret = -ENOMEM;
+		devpriv->dma_buffer[0] = pci_alloc_consistent(NULL,
+							      DAS16_DMA_SIZE,
+							      &devpriv->dma_buffer_addr[0]);
+		if (!devpriv->dma_buffer[0])
+			goto out_free_irq;
+
+		devpriv->dma_buffer[1] = pci_alloc_consistent(NULL,
+							      DAS16_DMA_SIZE,
+							      &devpriv->dma_buffer_addr[1]);
+		if (!devpriv->dma_buffer[1])
+			goto out_free_dma_buffers_0;
+
+		devpriv->dma_buffer[2] = pci_alloc_consistent(NULL,
+							      DAS16_DMA_SIZE,
+							      &devpriv->dma_buffer_addr[2]);
+		if (!devpriv->dma_buffer[2])
+			goto out_free_dma_buffers_1;
 
-			if (devpriv->dma_buffer[i] == NULL)
-				return -ENOMEM;
-		}
 		if (request_dma(dma_chan, "das16")) {
 			printk(KERN_ERR " failed to allocate dma channel %i\n",
 			       dma_chan);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out_free_dma_buffers_2;
 		}
 		devpriv->dma_chan = dma_chan;
 		flags = claim_dma_lock();
@@ -1531,7 +1542,8 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 		printk(KERN_INFO " ( no dma )\n");
 	} else {
 		printk(KERN_ERR " invalid dma channel\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_irq;
 	}
 
 	/*  get any user-defined input range */
@@ -1541,6 +1553,10 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 		devpriv->user_ai_range_table =
 		    kmalloc(sizeof(struct comedi_lrange) +
 			    sizeof(struct comedi_krange), GFP_KERNEL);
+		if (!devpriv->user_ai_range_table) {
+			ret = -ENOMEM;
+			goto out_free_dma;
+		}
 		/*  initialize ai range */
 		devpriv->user_ai_range_table->length = 1;
 		user_ai_range = devpriv->user_ai_range_table->range;
@@ -1554,6 +1570,10 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 		devpriv->user_ao_range_table =
 		    kmalloc(sizeof(struct comedi_lrange) +
 			    sizeof(struct comedi_krange), GFP_KERNEL);
+		if (!devpriv->user_ao_range_table) {
+			ret = -ENOMEM;
+			goto out_free_user_ai_range_table;
+		}
 		/*  initialize ao range */
 		devpriv->user_ao_range_table->length = 1;
 		user_ao_range = devpriv->user_ao_range_table->range;
@@ -1571,7 +1591,7 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	ret = alloc_subdevices(dev, 5);
 	if (ret < 0)
-		return ret;
+		goto out_free_user_ao_range_table;
 
 	s = dev->subdevices + 0;
 	dev->read_subdev = s;
@@ -1673,6 +1693,28 @@ static int das16_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	}
 
 	return 0;
+
+ out_free_user_ao_range_table:
+	kfree(devpriv->user_ao_range_table);
+ out_free_user_ai_range_table:
+	kfree(devpriv->user_ai_range_table);
+ out_free_dma:
+	if (devpriv->dma_chan)
+		free_dma(devpriv->dma_chan);
+ out_free_dma_buffers_2:
+	pci_free_consistent(NULL, DAS16_DMA_SIZE, devpriv->dma_buffer[2],
+			    devpriv->dma_buffer_addr[2]);
+ out_free_dma_buffers_1:
+	pci_free_consistent(NULL, DAS16_DMA_SIZE, devpriv->dma_buffer[1],
+			    devpriv->dma_buffer_addr[1]);
+ out_free_dma_buffers_0:
+	pci_free_consistent(NULL, DAS16_DMA_SIZE, devpriv->dma_buffer[0],
+			    devpriv->dma_buffer_addr[0]);
+ out_free_irq:
+	if (irq > 1 && irq < 8)
+		free_irq(dev->irq, dev);
+
+	return ret;
 }
 
 static int das16_detach(struct comedi_device *dev)



-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] staging, comedi, das16: das16_attach() needs to free allocated resources on failure

[Frank Mori Hess]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

On Sunday 07 November 2010, Jesper Juhl wrote:
> Hi,
>
> In drivers/staging/comedi/drivers/das16.c::das16_attach() there are many
> potential failure scenarios. However, the driver neglects to free
> resources it has allocated if failures happen.

This patch appears to be wrong.  The driver's detach function will be 
called by the core if the attach function returns with an error.  The 
resources are freed there.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] staging, comedi, das16: das16_attach() needs to free allocated resources on failure

[Jesper Juhl]

On Sun, 7 Nov 2010, Frank Mori Hess wrote:

> On Sunday 07 November 2010, Jesper Juhl wrote:
> > Hi,
> >
> > In drivers/staging/comedi/drivers/das16.c::das16_attach() there are many
> > potential failure scenarios. However, the driver neglects to free
> > resources it has allocated if failures happen.
> 
> This patch appears to be wrong.  The driver's detach function will be 
> called by the core if the attach function returns with an error.  The 
> resources are freed there.
> 
Ohh, sorry, I did not realize that.
In that case, please disregard this patch.
 

-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.