Patchwork [4/7,v2] GRETH: fixed skb buffer memory leak on frame errors

login
register
mail settings
Submitter Daniel Hellstrom
Date Jan. 14, 2011, 1:02 p.m.
Message ID <1295010163-2585-4-git-send-email-daniel@gaisler.com>
Download mbox | patch
Permalink /patch/78894/
State Accepted
Delegated to: David Miller
Headers show

Comments

Daniel Hellstrom - Jan. 14, 2011, 1:02 p.m.
A new SKB buffer should not be allocated when the old SKB is reused.

Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
---
 drivers/net/greth.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)
David Miller - Jan. 14, 2011, 8:46 p.m.
From: Daniel Hellstrom <daniel@gaisler.com>
Date: Fri, 14 Jan 2011 14:02:40 +0100

> A new SKB buffer should not be allocated when the old SKB is reused.
> 
> Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/greth.c b/drivers/net/greth.c
index 869e38d..9386bce 100644
--- a/drivers/net/greth.c
+++ b/drivers/net/greth.c
@@ -870,10 +870,9 @@  static int greth_rx_gbit(struct net_device *dev, int limit)
 			}
 		}
 
-		/* Allocate new skb to replace current */
-		newskb = netdev_alloc_skb(dev, MAX_FRAME_SIZE + NET_IP_ALIGN);
-
-		if (!bad && newskb) {
+		/* Allocate new skb to replace current, not needed if the
+		 * current skb can be reused */
+		if (!bad && (newskb=netdev_alloc_skb(dev, MAX_FRAME_SIZE + NET_IP_ALIGN))) {
 			skb_reserve(newskb, NET_IP_ALIGN);
 
 			dma_addr = dma_map_single(greth->dev,
@@ -910,11 +909,22 @@  static int greth_rx_gbit(struct net_device *dev, int limit)
 				if (net_ratelimit())
 					dev_warn(greth->dev, "Could not create DMA mapping, dropping packet\n");
 				dev_kfree_skb(newskb);
+				/* reusing current skb, so it is a drop */
 				dev->stats.rx_dropped++;
 			}
+		} else if (bad) {
+			/* Bad Frame transfer, the skb is reused */
+			dev->stats.rx_dropped++;
 		} else {
+			/* Failed Allocating a new skb. This is rather stupid
+			 * but the current "filled" skb is reused, as if
+			 * transfer failure. One could argue that RX descriptor
+			 * table handling should be divided into cleaning and
+			 * filling as the TX part of the driver
+			 */
 			if (net_ratelimit())
 				dev_warn(greth->dev, "Could not allocate SKB, dropping packet\n");
+			/* reusing current skb, so it is a drop */
 			dev->stats.rx_dropped++;
 		}