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

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

[Till Immanuel Patzschke]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

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: tip@internetwork-ag.de
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



[-- Attachment #2: qq2 --]
[-- Type: text/plain, Size: 1092 bytes --]

diff -Naur linux-2.4.10/drivers/net/ppp_generic.c linux-2.4.10-new/drivers/net/ppp_generic.c
--- linux-2.4.10/drivers/net/ppp_generic.c Sun Sep  9 19:45:43 2001
+++ linux-2.4.10-new/drivers/net/ppp_generic.c Tue Nov 13 21:48:29 2001
@@ -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,

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm and vc-encaps

[David S. Miller]

   From: Till Immanuel Patzschke <tip@internetwork-ag.de>
   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
davem@redhat.com

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm andvc-encaps

[Till Immanuel Patzschke]

"David S. Miller" wrote:

>    From: Till Immanuel Patzschke <tip@internetwork-ag.de>
>    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.

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: tip@internetwork-ag.de
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

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm andvc-encaps

[David S. Miller]

   From: Till Immanuel Patzschke <tip@internetwork-ag.de>
   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
davem@redhat.com

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm and vc-encaps

[Michal Ostrowski]

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.



> 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
> davem@redhat.com


-- 
Michal Ostrowski
mostrows@speakeasy.net

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm and vc-encaps

[David S. Miller]

   From: Michal Ostrowski <mostrows@speakeasy.net>
   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.

Re: [PATCH] ppp_generic causes skput:under: w/ pppoatm and vc-encaps

[Till Immanuel Patzschke]

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...
Michal Ostrowski wrote:

> 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.
>
> > 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
> > davem@redhat.com
>
> --
> Michal Ostrowski
> mostrows@speakeasy.net

--
Till Immanuel Patzschke                 mailto: tip@internetwork-ag.de
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