* [PATCH][net-next v1] gianfar: Fix alloc_skb_resources on -ENOMEM cleanup path
@ 2012-11-08 15:59 Claudiu Manoil
2012-11-08 16:08 ` Paul Gortmaker
0 siblings, 1 reply; 2+ messages in thread
From: Claudiu Manoil @ 2012-11-08 15:59 UTC (permalink / raw)
To: netdev; +Cc: Paul Gortmaker, David S. Miller, Claudiu Manoil
Should gfar_init_bds() return with -ENOMEM inside gfar_alloc_skb_resources(),
free_skb_resources() will be called twice in a row on the "cleanup" path,
leading to duplicate kfree() calls for rx_|tx_queue->rx_|tx_skbuff resulting
in segmentation fault.
This patch prevents the segmentation fault to happen in the future
(rx_|tx_sbkbuff set to NULL), and corrects the error path handling
for gfar_init_bds().
Cc: Paul Gortmaker <paul.gortmaker@windriver•com>
Cc: "David S. Miller" <davem@davemloft•net>
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale•com>
---
v1: do free_skb_resources() on the cleanup path of gfar_init_bds()'s parent
drivers/net/ethernet/freescale/gianfar.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 1d03dcd..ce2007f 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -210,7 +210,7 @@ static int gfar_init_bds(struct net_device *ndev)
skb = gfar_new_skb(ndev);
if (!skb) {
netdev_err(ndev, "Can't allocate RX buffers\n");
- goto err_rxalloc_fail;
+ return -ENOMEM;
}
rx_queue->rx_skbuff[j] = skb;
@@ -223,10 +223,6 @@ static int gfar_init_bds(struct net_device *ndev)
}
return 0;
-
-err_rxalloc_fail:
- free_skb_resources(priv);
- return -ENOMEM;
}
static int gfar_alloc_skb_resources(struct net_device *ndev)
@@ -1356,7 +1352,9 @@ static int gfar_restore(struct device *dev)
if (!netif_running(ndev))
return 0;
- gfar_init_bds(ndev);
+ if (gfar_init_bds(ndev))
+ goto cleanup;
+
init_registers(ndev);
gfar_set_mac_address(ndev);
gfar_init_mac(ndev);
@@ -1373,6 +1371,10 @@ static int gfar_restore(struct device *dev)
enable_napi(priv);
return 0;
+
+cleanup:
+ free_skb_resources(priv);
+ return -ENOMEM;
}
static struct dev_pm_ops gfar_pm_ops = {
@@ -1709,6 +1711,7 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue)
tx_queue->tx_skbuff[i] = NULL;
}
kfree(tx_queue->tx_skbuff);
+ tx_queue->tx_skbuff = NULL;
}
static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
@@ -1732,6 +1735,7 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
rxbdp++;
}
kfree(rx_queue->rx_skbuff);
+ rx_queue->rx_skbuff = NULL;
}
/* If there are any tx skbs or rx skbs still around, free them.
--
1.6.6
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH][net-next v1] gianfar: Fix alloc_skb_resources on -ENOMEM cleanup path
2012-11-08 15:59 [PATCH][net-next v1] gianfar: Fix alloc_skb_resources on -ENOMEM cleanup path Claudiu Manoil
@ 2012-11-08 16:08 ` Paul Gortmaker
0 siblings, 0 replies; 2+ messages in thread
From: Paul Gortmaker @ 2012-11-08 16:08 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S. Miller
On 12-11-08 10:59 AM, Claudiu Manoil wrote:
> Should gfar_init_bds() return with -ENOMEM inside gfar_alloc_skb_resources(),
> free_skb_resources() will be called twice in a row on the "cleanup" path,
> leading to duplicate kfree() calls for rx_|tx_queue->rx_|tx_skbuff resulting
> in segmentation fault.
> This patch prevents the segmentation fault to happen in the future
> (rx_|tx_sbkbuff set to NULL), and corrects the error path handling
> for gfar_init_bds().
>
> Cc: Paul Gortmaker <paul.gortmaker@windriver•com>
> Cc: "David S. Miller" <davem@davemloft•net>
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale•com>
> ---
> v1: do free_skb_resources() on the cleanup path of gfar_init_bds()'s parent
>
> drivers/net/ethernet/freescale/gianfar.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 1d03dcd..ce2007f 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -210,7 +210,7 @@ static int gfar_init_bds(struct net_device *ndev)
> skb = gfar_new_skb(ndev);
> if (!skb) {
> netdev_err(ndev, "Can't allocate RX buffers\n");
> - goto err_rxalloc_fail;
> + return -ENOMEM;
> }
> rx_queue->rx_skbuff[j] = skb;
>
> @@ -223,10 +223,6 @@ static int gfar_init_bds(struct net_device *ndev)
> }
>
> return 0;
> -
> -err_rxalloc_fail:
> - free_skb_resources(priv);
> - return -ENOMEM;
> }
>
> static int gfar_alloc_skb_resources(struct net_device *ndev)
> @@ -1356,7 +1352,9 @@ static int gfar_restore(struct device *dev)
> if (!netif_running(ndev))
> return 0;
>
> - gfar_init_bds(ndev);
> + if (gfar_init_bds(ndev))
> + goto cleanup;
> +
Since this is the only exit point, just put the free and
return right here, and avoid the goto and the separate
unwind block entirely?
P.
--
> init_registers(ndev);
> gfar_set_mac_address(ndev);
> gfar_init_mac(ndev);
> @@ -1373,6 +1371,10 @@ static int gfar_restore(struct device *dev)
> enable_napi(priv);
>
> return 0;
> +
> +cleanup:
> + free_skb_resources(priv);
> + return -ENOMEM;
> }
>
> static struct dev_pm_ops gfar_pm_ops = {
> @@ -1709,6 +1711,7 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue)
> tx_queue->tx_skbuff[i] = NULL;
> }
> kfree(tx_queue->tx_skbuff);
> + tx_queue->tx_skbuff = NULL;
> }
>
> static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
> @@ -1732,6 +1735,7 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
> rxbdp++;
> }
> kfree(rx_queue->rx_skbuff);
> + rx_queue->rx_skbuff = NULL;
> }
>
> /* If there are any tx skbs or rx skbs still around, free them.
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-11-08 16:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-08 15:59 [PATCH][net-next v1] gianfar: Fix alloc_skb_resources on -ENOMEM cleanup path Claudiu Manoil
2012-11-08 16:08 ` Paul Gortmaker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox