netdev - RE: Gianfar: RX Recycle skb->len error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <A6A1774AFD79E346AE6D49A33CB294530DC19F18@EX-BE-017-SFO.shared.themessagecenter.com>
Date:	Mon, 22 Mar 2010 14:10:48 -0700
From:	"Ben Menchaca (ben@...footnetworks.com)" <ben@...footnetworks.com>
To:	"avorontsov@...mvista.com" <avorontsov@...mvista.com>,
	"David Miller" <davem@...emloft.net>
cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Sandeep.Kumar@...escale.com" <Sandeep.Kumar@...escale.com>
Subject: RE: Gianfar: RX Recycle skb->len error

> Yes, skb_unreserve() (or skb_reset_reserved() for naming consistency?)
> would be great.

Just a couple of notes:
	It's yucky, but skb_reserve(skb, -alignamount) works, if you know the alignamount (which I discuss a bit below).  If that's not intended, then should len be unsigned int?  skb_put, skb_push, and skb_pull all seem to be unsigned int.

	It seems in both these cases for gianfar, the amount of the alignment is not immediately available to the code the recognizes that the skb_reset_reserved() is required.  A bit larger rework appears to be needed.  

I took a shot at using a new heuristic for the rx_ring to prevent having to skb_reset_reserved() at all.  The idea is to guarantee that the elements in ring are reserve()-ed, and that if we encounter an error condition that yields the two-skb case, free the new skb, since it has not yet been reserve()-ed.  Looking forward to hearing from FS on the "right" way, though.

Ben Menchaca
Bigfoot Networks

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index b671555..82f8486 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -109,6 +109,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev);
 static void gfar_reset_task(struct work_struct *work);
 static void gfar_timeout(struct net_device *dev);
 static int gfar_close(struct net_device *dev);
+static void gfar_skb_reserve_aligned(struct sk_buff *skb);
 struct sk_buff *gfar_new_skb(struct net_device *dev);
 static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp,
 		struct sk_buff *skb);
@@ -214,6 +215,7 @@ static int gfar_init_bds(struct net_device *ndev)
 							ndev->name);
 					goto err_rxalloc_fail;
 				}
+				gfar_skb_reserve_aligned(ndev);
 				rx_queue->rx_skbuff[j] = skb;
 
 				gfar_new_rxbdp(rx_queue, rxbdp, skb);
@@ -2372,6 +2374,20 @@ static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp,
 }
 
 
+static void gfar_skb_reserve_aligned(struct sk_buff *skb)
+{
+	unsigned int alignamount;
+
+        alignamount = RXBUF_ALIGNMENT -
+                (((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1));
+
+        /* We need the data buffer to be aligned properly.  We will reserve
+         * as many bytes as needed to align the data properly
+         */
+        skb_reserve(skb, alignamount);
+}
+
+
 struct sk_buff * gfar_new_skb(struct net_device *dev)
 {
 	unsigned int alignamount;
@@ -2383,17 +2399,6 @@ struct sk_buff * gfar_new_skb(struct net_device *dev)
 		skb = netdev_alloc_skb(dev,
 				priv->rx_buffer_size + RXBUF_ALIGNMENT);
 
-	if (!skb)
-		return NULL;
-
-	alignamount = RXBUF_ALIGNMENT -
-		(((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1));
-
-	/* We need the data buffer to be aligned properly.  We will reserve
-	 * as many bytes as needed to align the data properly
-	 */
-	skb_reserve(skb, alignamount);
-
 	return skb;
 }
 
@@ -2532,21 +2537,17 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 			if (unlikely(!newskb))
 				newskb = skb;
 			else if (skb) {
-				/*
-				 * We need to reset ->data to what it
-				 * was before gfar_new_skb() re-aligned
-				 * it to an RXBUF_ALIGNMENT boundary
-				 * before we put the skb back on the
-				 * recycle list.
-				 */
-				skb->data = skb->head + NET_SKB_PAD;
-				__skb_queue_head(&priv->rx_recycle, skb);
+				__skb_queue_head(&priv->rx_recycle, newskb);
+				newskb = skb;
+			} else {
+				gfar_skb_reserve_aligned(newskb);
 			}
 		} else {
 			/* Increment the number of packets */
 			rx_queue->stats.rx_packets++;
 			howmany++;
 
+			gfar_skb_reserve_aligned(newskb);
 			if (likely(skb)) {
 				pkt_len = bdp->length - ETH_FCS_LEN;
 				/* Remove the FCS from the packet length */