Re: [PATCH] PPPOE can kfree SKB twice (was Re: kernel panic problem. (smp, iptables?))

[Michal Ostrowski <mostrows@us.ibm.com>]

After sleeping on it a bit I've come to the realization that are
still some issues outstanding.

Essentially, if __pppoe_xmit has been forced to make a copy of the skb
(because netfilter did not leave enough room for PPPoE headers), and
dev_queue_xmit fails, the copy of the skb is not freed and we have a
memory leak.

The generic PPP layer assumes that if a PPP-channel's xmit routine
fails then the skb is still available to it (for retransmission) and
otherwise the skb is gone -- handled by the channel.  These are the
semantics that must be implemented by __pppoe_xmit.

The patch below (which requires David Miller's patch from 17/07/01)
implements these semantics.

Michal Ostrowski
mostrows@speakeasy.net

--- drivers/net/pppoe.c~	Wed Jul 18 10:24:25 2001
+++ drivers/net/pppoe.c	Thu Jul 19 08:28:56 2001
@@ -5,7 +5,7 @@
  * PPPoE --- PPP over Ethernet (RFC 2516)
  *
  *
- * Version:    0.6.7
+ * Version:    0.6.8
  *
  * 030700 :     Fixed connect logic to allow for disconnect.
  * 270700 :	Fixed potential SMP problems; we must protect against
@@ -25,8 +25,12 @@
  *		locked. (DaveM)
  *		Ignore return value of dev_queue_xmit in __pppoe_xmit
  *		or else we may kfree an SKB twice. (DaveM)
+ * 190701 :	When doing copies of skb's in __pppoe_xmit, always delete
+ *		the original skb that was passed in on success, never on
+ *		failure.  Delete the copy of the skb on failure to avoid
+ *		a memory leak.
  *
- * Author:	Michal Ostrowski <mostrows@styx.uwaterloo.ca>
+ * Author:	Michal Ostrowski <mostrows@speakeasy.net>
  * Contributors:
  * 		Arnaldo Carvalho de Melo <acme@xconectiva.com.br>
  *		David S. Miller (davem@redhat.com)
@@ -849,6 +853,7 @@
 	struct pppoe_hdr *ph;
 	int headroom = skb_headroom(skb);
 	int data_len = skb->len;
+	struct sk_buff *old_skb = NULL;
 
 	if (sk->dead  || !(sk->state & PPPOX_CONNECTED))
 		goto abort;
@@ -876,17 +881,12 @@
 		skb_reserve(skb2, dev->hard_header_len + sizeof(struct pppoe_hdr));
 		memcpy(skb_put(skb2, skb->len), skb->data, skb->len);
 
-		skb_unlink(skb);
-		kfree_skb(skb);
+		/* Keep a reference to the original */
+		old_skb = skb;
+
 		skb = skb2;
 	}
 
-	/* We may not return error beyond this point.  Potentially this
-	 * is a new SKB and in such a case we've already freed the
-	 * original SKB.  So if we were to return error, our caller would
-	 * double free that original SKB. -DaveM
-	 */
-
 	ph = (struct pppoe_hdr *) skb_push(skb, sizeof(struct pppoe_hdr));
 	memcpy(ph, &hdr, sizeof(struct pppoe_hdr));
 	skb->protocol = __constant_htons(ETH_P_PPP_SES);
@@ -899,7 +899,34 @@
 			 sk->protinfo.pppox->pppoe_pa.remote,
 			 NULL, data_len);
 
-	dev_queue_xmit(skb);
+	/* The skb we are to transmit may be a copy (see above).  If
+	 * this fails, then the caller is responsible for the original
+	 * skb, otherwise we must free it.  Also if this fails we must
+	 * free the copy that we made.
+	 */
+
+	if (dev_queue_xmit(skb)<0) {
+		if (old_skb) {
+			/* The skb we tried to send was a copy.  We
+			 * have to free it (the copy) and let the
+			 * caller deal with the original one.
+			 */
+			skb_unlink(skb);
+			kfree_skb(skb);
+		}
+		goto abort;
+	}
+
+	/* Free original skb now that we know dev_queue_xmit
+	 * succeeded.  We free only "old_skb" because dev_queue_xmit
+	 * actually sent a copy, not the original.
+	 */
+
+	if (old_skb) {
+		skb_unlink(old_skb);
+		kfree_skb(old_skb);
+	}
+
 	return 1;
 
 abort:
@@ -1049,7 +1076,6 @@
  	int err = register_pppox_proto(PX_PROTO_OE, &pppoe_proto);
 
 	if (err == 0) {
-		printk(KERN_INFO "Registered PPPoE v0.6.5\n");
 
 		dev_add_pack(&pppoes_ptype);
 		dev_add_pack(&pppoed_ptype);
--- drivers/net/pppox.c~	Tue Feb 13 16:15:05 2001
+++ drivers/net/pppox.c	Wed Jul 18 10:27:25 2001
@@ -148,10 +148,6 @@
 
 	err = sock_register(&pppox_proto_family);
 
-	if (err == 0) {
-		printk(KERN_INFO "Registered PPPoX v0.5\n");
-	}
-
 	return err;
 }