* [PATCH] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
@ 2010-12-26 20:30 Jesper Juhl
2010-12-26 20:54 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2010-12-26 20:30 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, John(Zongxi) Chen, Michael Chan
Hi,
We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
either of the calls to dma_alloc_coherent() fail. This patch fixes it by
freeing both the memory allocated with kzalloc() and memory allocated with
previous calls to dma_alloc_coherent() when there's a failure.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
cnic.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
compile tested only since I don't have the hardware to do a proper test.
diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
index 92bac19..f094159 100644
--- a/drivers/net/cnic.c
+++ b/drivers/net/cnic.c
@@ -939,16 +939,22 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
udev->l2_ring = dma_alloc_coherent(&udev->pdev->dev, udev->l2_ring_size,
&udev->l2_ring_map,
GFP_KERNEL | __GFP_COMP);
- if (!udev->l2_ring)
+ if (!udev->l2_ring) {
+ kfree(udev);
return -ENOMEM;
+ }
udev->l2_buf_size = (cp->l2_rx_ring_size + 1) * cp->l2_single_buf_size;
udev->l2_buf_size = PAGE_ALIGN(udev->l2_buf_size);
udev->l2_buf = dma_alloc_coherent(&udev->pdev->dev, udev->l2_buf_size,
&udev->l2_buf_map,
GFP_KERNEL | __GFP_COMP);
- if (!udev->l2_buf)
+ if (!udev->l2_buf) {
+ dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
+ udev->l2_ring, udev->l2_ring_map);
+ kfree(udev);
return -ENOMEM;
+ }
write_lock(&cnic_dev_lock);
list_add(&udev->list, &cnic_udev_list);
--
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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
2010-12-26 20:30 [PATCH] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings() Jesper Juhl
@ 2010-12-26 20:54 ` Joe Perches
2010-12-26 20:57 ` [PATCH v2] " Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2010-12-26 20:54 UTC (permalink / raw)
To: Jesper Juhl; +Cc: netdev, linux-kernel, John(Zongxi) Chen, Michael Chan
On Sun, 2010-12-26 at 21:30 +0100, Jesper Juhl wrote:
> We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
> either of the calls to dma_alloc_coherent() fail.
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> cnic.c | 10 ++++++++--
> diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
[]
> - if (!udev->l2_ring)
> + if (!udev->l2_ring) {
> + kfree(udev);
> return -ENOMEM;
> + }
[]
> - if (!udev->l2_buf)
> + if (!udev->l2_buf) {
> + dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
> + udev->l2_ring, udev->l2_ring_map);
> + kfree(udev);
> return -ENOMEM;
> + }
Perhaps this would be more standard with a goto error / exit block
err_dma:
dma_free_coherent();
err_udev:
kfree(udev);
return -ENOMEM;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
2010-12-26 20:54 ` Joe Perches
@ 2010-12-26 20:57 ` Jesper Juhl
2010-12-27 3:57 ` Michael Chan
2010-12-31 19:20 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Jesper Juhl @ 2010-12-26 20:57 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, linux-kernel, John(Zongxi) Chen, Michael Chan
On Sun, 26 Dec 2010, Joe Perches wrote:
> On Sun, 2010-12-26 at 21:30 +0100, Jesper Juhl wrote:
> > We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
> > either of the calls to dma_alloc_coherent() fail.
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > cnic.c | 10 ++++++++--
> > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
> []
> > - if (!udev->l2_ring)
> > + if (!udev->l2_ring) {
> > + kfree(udev);
> > return -ENOMEM;
> > + }
> []
> > - if (!udev->l2_buf)
> > + if (!udev->l2_buf) {
> > + dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
> > + udev->l2_ring, udev->l2_ring_map);
> > + kfree(udev);
> > return -ENOMEM;
> > + }
>
> Perhaps this would be more standard with a goto error / exit block
>
> err_dma:
> dma_free_coherent();
> err_udev:
> kfree(udev);
> return -ENOMEM;
>
I have no problem with that. It's functionally the same, but the object
file size is smaller and, as you say, it's more standard. Good point,
thanks.
We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
either of the calls to dma_alloc_coherent() fail. This patch fixes it by
freeing both the memory allocated with kzalloc() and memory allocated with
previous calls to dma_alloc_coherent() when there's a failure.
Thanks to Joe Perches <joe@perches.com> for suggesting a better
implementation than my initial version.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
cnic.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
compile tested only since I don't have the hardware to do a proper test.
diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
index 92bac19..952afac 100644
--- a/drivers/net/cnic.c
+++ b/drivers/net/cnic.c
@@ -940,7 +940,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
&udev->l2_ring_map,
GFP_KERNEL | __GFP_COMP);
if (!udev->l2_ring)
- return -ENOMEM;
+ goto err_udev;
udev->l2_buf_size = (cp->l2_rx_ring_size + 1) * cp->l2_single_buf_size;
udev->l2_buf_size = PAGE_ALIGN(udev->l2_buf_size);
@@ -948,7 +948,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
&udev->l2_buf_map,
GFP_KERNEL | __GFP_COMP);
if (!udev->l2_buf)
- return -ENOMEM;
+ goto err_dma;
write_lock(&cnic_dev_lock);
list_add(&udev->list, &cnic_udev_list);
@@ -959,6 +959,12 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
cp->udev = udev;
return 0;
+ err_dma:
+ dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
+ udev->l2_ring, udev->l2_ring_map);
+ err_udev:
+ kfree(udev);
+ return -ENOMEM;
}
static int cnic_init_uio(struct cnic_dev *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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
2010-12-26 20:57 ` [PATCH v2] " Jesper Juhl
@ 2010-12-27 3:57 ` Michael Chan
2010-12-31 19:20 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Michael Chan @ 2010-12-27 3:57 UTC (permalink / raw)
To: 'Jesper Juhl', 'Joe Perches',
'davem@davemloft.net'
Cc: 'netdev@vger.kernel.org',
'linux-kernel@vger.kernel.org',
'John(Zongxi) Chen'
Jesper Juhl wrote:
>
> We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
> either of the calls to dma_alloc_coherent() fail. This patch fixes it
> by
> freeing both the memory allocated with kzalloc() and memory allocated
> with
> previous calls to dma_alloc_coherent() when there's a failure.
>
> Thanks to Joe Perches <joe@perches.com> for suggesting a better
> implementation than my initial version.
>
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Thanks.
Acked-by: Michael Chan <mchan@broadcom.com>
> ---
> cnic.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> compile tested only since I don't have the hardware to do a proper
> test.
>
> diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
> index 92bac19..952afac 100644
> --- a/drivers/net/cnic.c
> +++ b/drivers/net/cnic.c
> @@ -940,7 +940,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev
> *dev, int pages)
> &udev->l2_ring_map,
> GFP_KERNEL | __GFP_COMP);
> if (!udev->l2_ring)
> - return -ENOMEM;
> + goto err_udev;
>
> udev->l2_buf_size = (cp->l2_rx_ring_size + 1) * cp-
> >l2_single_buf_size;
> udev->l2_buf_size = PAGE_ALIGN(udev->l2_buf_size);
> @@ -948,7 +948,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev
> *dev, int pages)
> &udev->l2_buf_map,
> GFP_KERNEL | __GFP_COMP);
> if (!udev->l2_buf)
> - return -ENOMEM;
> + goto err_dma;
>
> write_lock(&cnic_dev_lock);
> list_add(&udev->list, &cnic_udev_list);
> @@ -959,6 +959,12 @@ static int cnic_alloc_uio_rings(struct cnic_dev
> *dev, int pages)
> cp->udev = udev;
>
> return 0;
> + err_dma:
> + dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
> + udev->l2_ring, udev->l2_ring_map);
> + err_udev:
> + kfree(udev);
> + return -ENOMEM;
> }
>
> static int cnic_init_uio(struct cnic_dev *dev)
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
2010-12-26 20:57 ` [PATCH v2] " Jesper Juhl
2010-12-27 3:57 ` Michael Chan
@ 2010-12-31 19:20 ` David Miller
2011-01-02 18:54 ` Jesper Juhl
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2010-12-31 19:20 UTC (permalink / raw)
To: jj; +Cc: joe, netdev, linux-kernel, zongxi, mchan
From: Jesper Juhl <jj@chaosbits.net>
Date: Sun, 26 Dec 2010 21:57:39 +0100 (CET)
> We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
> either of the calls to dma_alloc_coherent() fail. This patch fixes it by
> freeing both the memory allocated with kzalloc() and memory allocated with
> previous calls to dma_alloc_coherent() when there's a failure.
>
> Thanks to Joe Perches <joe@perches.com> for suggesting a better
> implementation than my initial version.
>
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
...
> + err_dma:
> + dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
> + udev->l2_ring, udev->l2_ring_map);
Space before tab in indentation, improperly positioned third argument
indentation.
I fixed all of this up, but please do not skimp on making sure these
details are taken care of when updating your patch in response to feedback.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings()
2010-12-31 19:20 ` David Miller
@ 2011-01-02 18:54 ` Jesper Juhl
0 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2011-01-02 18:54 UTC (permalink / raw)
To: David Miller; +Cc: joe, netdev, linux-kernel, zongxi, mchan
On Fri, 31 Dec 2010, David Miller wrote:
> From: Jesper Juhl <jj@chaosbits.net>
> Date: Sun, 26 Dec 2010 21:57:39 +0100 (CET)
>
> > We are leaking memory in drivers/net/cnic.c::cnic_alloc_uio_rings() if
> > either of the calls to dma_alloc_coherent() fail. This patch fixes it by
> > freeing both the memory allocated with kzalloc() and memory allocated with
> > previous calls to dma_alloc_coherent() when there's a failure.
> >
> > Thanks to Joe Perches <joe@perches.com> for suggesting a better
> > implementation than my initial version.
> >
> >
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
>
> ...
> > + err_dma:
> > + dma_free_coherent(&udev->pdev->dev, udev->l2_ring_size,
> > + udev->l2_ring, udev->l2_ring_map);
>
> Space before tab in indentation, improperly positioned third argument
> indentation.
>
Whoops.
> I fixed all of this up, but please do not skimp on making sure these
> details are taken care of when updating your patch in response to feedback.
>
I usually try to take care that such things are in order. I often even
point these details out to other people. It slipped past me this time.
That was a mistake. Sorry.
--
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-02 18:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-26 20:30 [PATCH] Broadcom CNIC core network driver: fix mem leak on allocation failures in cnic_alloc_uio_rings() Jesper Juhl
2010-12-26 20:54 ` Joe Perches
2010-12-26 20:57 ` [PATCH v2] " Jesper Juhl
2010-12-27 3:57 ` Michael Chan
2010-12-31 19:20 ` David Miller
2011-01-02 18:54 ` Jesper Juhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).