* [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