* [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() @ 2015-01-15 1:43 Kukjin Kim 2015-01-15 8:12 ` Dan Carpenter 2015-01-16 0:14 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Kukjin Kim @ 2015-01-15 1:43 UTC (permalink / raw) To: netdev; +Cc: Dave Miller, Dan Carpenter, Byungho An, Kukjin Kim From: Byungho An <bh74.an@samsung.com> This patch fixes double kfree() calls at init_rx_ring() because it causes static checker warning. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Byungho An <bh74.an@samsung.com> Signed-off-by: Kukjin Kim <kgene@kernel.org> --- drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c index 6984944..fe7538d 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -474,13 +474,19 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, /* allocate memory for RX skbuff array */ rx_ring->rx_skbuff_dma = kmalloc_array(rx_rsize, sizeof(dma_addr_t), GFP_KERNEL); - if (rx_ring->rx_skbuff_dma == NULL) - goto dmamem_err; + if (!rx_ring->rx_skbuff_dma) { + dma_free_coherent(priv->device, + rx_rsize * sizeof(struct sxgbe_rx_norm_desc), + rx_ring->dma_rx, rx_ring->dma_rx_phy); + goto error; + } rx_ring->rx_skbuff = kmalloc_array(rx_rsize, sizeof(struct sk_buff *), GFP_KERNEL); - if (rx_ring->rx_skbuff == NULL) - goto rxbuff_err; + if (!rx_ring->rx_skbuff) { + kfree(rx_ring->rx_skbuff_dma); + goto error; + } /* initialise the buffers */ for (desc_index = 0; desc_index < rx_rsize; desc_index++) { @@ -502,13 +508,6 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, err_init_rx_buffers: while (--desc_index >= 0) free_rx_ring(priv->device, rx_ring, desc_index); - kfree(rx_ring->rx_skbuff); -rxbuff_err: - kfree(rx_ring->rx_skbuff_dma); -dmamem_err: - dma_free_coherent(priv->device, - rx_rsize * sizeof(struct sxgbe_rx_norm_desc), - rx_ring->dma_rx, rx_ring->dma_rx_phy); error: return -ENOMEM; } -- 2.0.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() 2015-01-15 1:43 [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() Kukjin Kim @ 2015-01-15 8:12 ` Dan Carpenter 2015-01-16 0:14 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2015-01-15 8:12 UTC (permalink / raw) To: Kukjin Kim; +Cc: netdev, Dave Miller, Byungho An On Thu, Jan 15, 2015 at 10:43:11AM +0900, Kukjin Kim wrote: > rx_ring->rx_skbuff = kmalloc_array(rx_rsize, > sizeof(struct sk_buff *), GFP_KERNEL); > - if (rx_ring->rx_skbuff == NULL) > - goto rxbuff_err; > + if (!rx_ring->rx_skbuff) { This is missing a dma_free_coherent() here. > + kfree(rx_ring->rx_skbuff_dma); > + goto error; > + } > > /* initialise the buffers */ > for (desc_index = 0; desc_index < rx_rsize; desc_index++) { > @@ -502,13 +508,6 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, > err_init_rx_buffers: > while (--desc_index >= 0) > free_rx_ring(priv->device, rx_ring, desc_index); > - kfree(rx_ring->rx_skbuff); The double free bug is that free_rx_ring() frees kfree(rx_ring->rx_skbuff_dma); and kfree(rx_ring->rx_skbuff); so just calling it in a loop here will cause a double free. Also free_rx_ring() doesn't take an index parameter, it takes a size parameter. The right way to fix this is to create a release function that mirrors sxgbe_init_rx_buffers(). static void sxgbe_free_rx_buffers(struct net_device *dev, struct sxgbe_rx_norm_desc *p, int i, unsigned int dma_buf_sz, struct sxgbe_rx_queue *rx_ring) { struct sxgbe_priv_data *priv = netdev_priv(dev); kfree_skb(rx_ring->rx_skbuff[i]); dma_unmap_single(priv->device, rx_ring->rx_skbuff_dma[i], dma_buf_sz, DMA_FROM_DEVICE); } Then just swap that into the free loop instead of the free_rx_ring(). err_init_rx_buffers: while (--desc_index >= 0) { struct sxgbe_rx_norm_desc *p; p = rx_ring->dma_rx + desc_index; sxgbe_init_rx_buffers(dev, p, desc_index, bfsize, rx_ring); } kfree(rx_ring->rx_skbuff); Something like that. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() 2015-01-15 1:43 [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() Kukjin Kim 2015-01-15 8:12 ` Dan Carpenter @ 2015-01-16 0:14 ` David Miller 2015-01-16 7:12 ` Dan Carpenter 1 sibling, 1 reply; 6+ messages in thread From: David Miller @ 2015-01-16 0:14 UTC (permalink / raw) To: kgene; +Cc: netdev, davem, dan.carpenter, bh74.an From: Kukjin Kim <kgene@kernel.org> Date: Thu, 15 Jan 2015 10:43:11 +0900 > From: Byungho An <bh74.an@samsung.com> > > This patch fixes double kfree() calls at init_rx_ring() because > it causes static checker warning. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Byungho An <bh74.an@samsung.com> > Signed-off-by: Kukjin Kim <kgene@kernel.org> Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() 2015-01-16 0:14 ` David Miller @ 2015-01-16 7:12 ` Dan Carpenter 2015-02-05 8:00 ` [patch] net: sxgbe: fix error handling in init_rx_ring() Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2015-01-16 7:12 UTC (permalink / raw) To: David Miller; +Cc: kgene, netdev, davem, bh74.an On Thu, Jan 15, 2015 at 07:14:16PM -0500, David Miller wrote: > From: Kukjin Kim <kgene@kernel.org> > Date: Thu, 15 Jan 2015 10:43:11 +0900 > > > From: Byungho An <bh74.an@samsung.com> > > > > This patch fixes double kfree() calls at init_rx_ring() because > > it causes static checker warning. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Byungho An <bh74.an@samsung.com> > > Signed-off-by: Kukjin Kim <kgene@kernel.org> > > Applied. It does silence the warning but it doesn't fix the bug. Kukjin, let me know if you have any questions. I can write the fix if you need me to. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch] net: sxgbe: fix error handling in init_rx_ring() 2015-01-16 7:12 ` Dan Carpenter @ 2015-02-05 8:00 ` Dan Carpenter 2015-02-06 20:50 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2015-02-05 8:00 UTC (permalink / raw) To: Byungho An, kgene; +Cc: Girish K S, Vipul Pandya, netdev, kernel-janitors There are a couple bugs with the error handling in this function. 1) If we can't allocate "rx_ring->rx_skbuff" then we should call dma_free_coherent() but we don't. 2) free_rx_ring() frees "rx_ring->rx_skbuff_dma" and "rx_ring->rx_skbuff" so calling it in a loop causes a double free. Also it was a bit confusing how we sometimes freed things before doing the goto. I've cleaned it up so it does error handling in normal kernel style. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c index 11288d4..c8a01ee 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -364,6 +364,26 @@ static int sxgbe_init_rx_buffers(struct net_device *dev, return 0; } + +/** + * sxgbe_free_rx_buffers - free what sxgbe_init_rx_buffers() allocated + * @dev: net device structure + * @rx_ring: ring to be freed + * @rx_rsize: ring size + * Description: this function initializes the DMA RX descriptor + */ +static void sxgbe_free_rx_buffers(struct net_device *dev, + struct sxgbe_rx_norm_desc *p, int i, + unsigned int dma_buf_sz, + struct sxgbe_rx_queue *rx_ring) +{ + struct sxgbe_priv_data *priv = netdev_priv(dev); + + kfree_skb(rx_ring->rx_skbuff[i]); + dma_unmap_single(priv->device, rx_ring->rx_skbuff_dma[i], + dma_buf_sz, DMA_FROM_DEVICE); +} + /** * init_tx_ring - init the TX descriptor ring * @dev: net device structure @@ -456,7 +476,7 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, /* RX ring is not allcoated */ if (rx_ring == NULL) { netdev_err(dev, "No memory for RX queue\n"); - goto error; + return -ENOMEM; } /* assign queue number */ @@ -468,23 +488,21 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, &rx_ring->dma_rx_phy, GFP_KERNEL); if (rx_ring->dma_rx == NULL) - goto error; + return -ENOMEM; /* allocate memory for RX skbuff array */ rx_ring->rx_skbuff_dma = kmalloc_array(rx_rsize, sizeof(dma_addr_t), GFP_KERNEL); if (!rx_ring->rx_skbuff_dma) { - dma_free_coherent(priv->device, - rx_rsize * sizeof(struct sxgbe_rx_norm_desc), - rx_ring->dma_rx, rx_ring->dma_rx_phy); - goto error; + ret = -ENOMEM; + goto err_free_dma_rx; } rx_ring->rx_skbuff = kmalloc_array(rx_rsize, sizeof(struct sk_buff *), GFP_KERNEL); if (!rx_ring->rx_skbuff) { - kfree(rx_ring->rx_skbuff_dma); - goto error; + ret = -ENOMEM; + goto err_free_skbuff_dma; } /* initialise the buffers */ @@ -494,7 +512,7 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, ret = sxgbe_init_rx_buffers(dev, p, desc_index, bfsize, rx_ring); if (ret) - goto err_init_rx_buffers; + goto err_free_rx_buffers; } /* initalise counters */ @@ -504,11 +522,22 @@ static int init_rx_ring(struct net_device *dev, u8 queue_no, return 0; -err_init_rx_buffers: - while (--desc_index >= 0) - free_rx_ring(priv->device, rx_ring, desc_index); -error: - return -ENOMEM; +err_free_rx_buffers: + while (--desc_index >= 0) { + struct sxgbe_rx_norm_desc *p; + + p = rx_ring->dma_rx + desc_index; + sxgbe_free_rx_buffers(dev, p, desc_index, bfsize, rx_ring); + } + kfree(rx_ring->rx_skbuff); +err_free_skbuff_dma: + kfree(rx_ring->rx_skbuff_dma); +err_free_dma_rx: + dma_free_coherent(priv->device, + rx_rsize * sizeof(struct sxgbe_rx_norm_desc), + rx_ring->dma_rx, rx_ring->dma_rx_phy); + + return ret; } /** * free_tx_ring - free the TX descriptor ring ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] net: sxgbe: fix error handling in init_rx_ring() 2015-02-05 8:00 ` [patch] net: sxgbe: fix error handling in init_rx_ring() Dan Carpenter @ 2015-02-06 20:50 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2015-02-06 20:50 UTC (permalink / raw) To: dan.carpenter Cc: bh74.an, kgene, ks.giri, vipul.pandya, netdev, kernel-janitors From: Dan Carpenter <dan.carpenter@oracle.com> Date: Thu, 5 Feb 2015 11:00:42 +0300 > There are a couple bugs with the error handling in this function. > > 1) If we can't allocate "rx_ring->rx_skbuff" then we should call > dma_free_coherent() but we don't. > 2) free_rx_ring() frees "rx_ring->rx_skbuff_dma" and "rx_ring->rx_skbuff" > so calling it in a loop causes a double free. > > Also it was a bit confusing how we sometimes freed things before doing > the goto. I've cleaned it up so it does error handling in normal kernel > style. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Looks good, applied, thanks Dan. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-06 20:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-15 1:43 [PATCH] [PATCH] net: sxgbe: Fix waring for double kfree() Kukjin Kim 2015-01-15 8:12 ` Dan Carpenter 2015-01-16 0:14 ` David Miller 2015-01-16 7:12 ` Dan Carpenter 2015-02-05 8:00 ` [patch] net: sxgbe: fix error handling in init_rx_ring() Dan Carpenter 2015-02-06 20:50 ` David Miller
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).