ppp_generic causes skput:under: w/ pppoatm and vc-encaps

Hi,

using pppoatm from Mitchell Blank jr resulted in a BUG "skput:under: ..." when used w/ vc-encapsulation. The problem is (at least w/ the iphase driver) that the skb's headroom is 0 (since the vc-encaps. is nothing but the PPP frame) and ppp_receive_nonmp_frame unconditionally calls skb_push -> BUG(skput:under: ...") in the CONFIG_PPP_FILTER section.

I've attached a patch, checking for headroom first, and - if necessary - reallocating a larger buffer for the skb_push.

Please check and apply - or find a better fix!

Thanks,

Immanuel

--

Till Immanuel Patzschke                 mailto: [email protected]
interNetwork AG                         Phone:  +49-(0)611-1731-121
Bierstadter Str. 7                      Fax:    +49-(0)611-1731-31
D-65189 Wiesbaden                       Web:    http://www.internetwork-ag.de

diff -Naur linux-2.4.10/drivers/net/ppp_generic.c linux-2.4.10-new/drivers/net/ppp_generic.c
@@ -1470,6 +1470,23 @@
   /* check if the packet passes the pass and active filters */
   /* the filter instructions are constructed assuming
      a four-byte PPP header on each packet */
+         /* make sure we have room for the following skb_push... */
+         if (skb_headroom(skb) < PPP_HDRLEN) {
+      struct sk_buff *ns;
+      
+      ns = alloc_skb(skb->len + PPP_HDRLEN*2,GFP_ATOMIC);
+      if (ns == 0) {
+   printk(KERN_DEBUG "PPP: inbound skb not resizeable.\n");
+   kfree_skb(skb);
+   return;
+      }
+      skb_reserve(ns, PPP_HDRLEN*2);
+      memcpy(skb_put(ns, skb->len), skb->data, skb->len);
+      kfree_skb(skb);
+      skb = ns;
+      if (ppp->debug & 1)
+   printk(KERN_DEBUG "PPP: inbound skb resized.\n");
+  }
   *skb_push(skb, 2) = 0;
   if (ppp->pass_filter.filter
       && sk_run_filter(skb, ppp->pass_filter.filter,

From: Till Immanuel Patzschke <[email protected]>
Date: Tue, 13 Nov 2001 22:30:24 +0100

I've attached a patch, checking for headroom first, and - if necessary - reallocating a larger buffer for the skb_push.

Please check and apply - or find a better fix!

Here is my "better fix". In pppoatm, we should be increasing the device header length appropriately. ie. dev->hard_header_len needs to be increased in the pppoatm driver when vc-encaps is used.

Franks a lot,
David S. Miller
[email protected]

If you set the hdrlen field of the ppp_channel that pppoatm registers (pvcc->chan.hdrlen, in pppoatm_assign_vcc()) then ppp_generic will always over-allocate skb space to allow for extra headers to be pushed in. This mechanism was put is so that we wouldn't have to copy the frame in order to slap on PPPoE headers onto it. I think it's a good idea to be doing this, especially if you're going to play with hard_header_len.

If you look at pppoatm_send(), you'll see that you do an
skb_realloc_headroom if there's no space for the headers. If
pvcc->chan.hdrlen is set properly then this will be the exceptional, rather than the common case.

Michal Ostrowski
[email protected]

From: Michal Ostrowski <[email protected]>
Date: 14 Nov 2001 09:23:53 -0500

If you look at pppoatm_send(), you'll see that you do an
skb_realloc_headroom if there's no space for the headers. If
pvcc->chan.hdrlen is set properly then this will be the exceptional, rather than the common case.

Their problem is on receive, not send. That's problematic because in that case the ATM drivers allocate and size the SKBs.

that is - of course - a more pppoatm specific fix BUT my concern ist that PPP requires (!) to have at least 2 bytes headroom [when using PPP_FILTER] (which is NOT noted), and isn't it good practice to check for room before claiming it?

--

Till Immanuel Patzschke                 mailto: [email protected]
interNetwork AG                         Phone:  +49-(0)611-1731-121
Bierstadter Str. 7                      Fax:    +49-(0)611-1731-31
D-65189 Wiesbaden                       Web:    http://www.internetwork-ag.de

From: Till Immanuel Patzschke <[email protected]>
Date: Wed, 14 Nov 2001 12:13:10 +0100

that is - of course - a more pppoatm specific fix BUT my concern ist that PPP requires (!) to have at least 2 bytes headroom [when using PPP_FILTER] (which is NOT noted), and isn't it good practice to check for room before claiming it?

Ignore my comments, I thought this was happening on output. Yes, it seems ppp_generic should be verifying the headroom.

Franks a lot,
David S. Miller
[email protected]

Hi Michal,

it is the receive side - (chan.hdrlen is set correctly) -- on the receive side you'll the SKB from the device AND headroom is up to the device...

--

Till Immanuel Patzschke                 mailto: [email protected]
interNetwork AG                         Phone:  +49-(0)611-1731-121
Bierstadter Str. 7                      Fax:    +49-(0)611-1731-31
D-65189 Wiesbaden                       Web:    http://www.internetwork-ag.de