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

From: Michal Ostrowski (mostrows@us.ibm.com)
Date: Thu Jul 19 2001 - 07:30:37 EST


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;
 }
 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Mon Jul 23 2001 - 21:00:12 EST