Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Patrick McHardy]

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

Bartek wrote:
> Hopefully, this time it my bug report should be ok :):
> 
> Apr 11 23:53:38 localhost pppd[31289]: rcvd [proto=0x7689] e1 cd 33 f6
> fd f7 52 e6 58 c9 73 98 bc ff ad d5 b5 a3 e5 d9 1e 77 76 0a 1c 87 59
> bf 44 cc ac 3b ...
> Apr 11 23:53:38 localhost pppd[31289]: Unsupported protocol 0x7689 received
> Apr 11 23:53:38 localhost pppd[31289]: sent [LCP ProtRej id=0x9 76 89
> e1 cd 33 f6 fd f7 52 e6 58 c9 73 98 bc ff ad d5 b5 a3 e5 d9 1e 77 76
> 0a 1c 87 59 bf 44 cc ...]
> Apr 11 23:53:38 localhost pppd[31289]: rcvd [proto=0xda7d] 15 19 45 3c
> e0 ac 44 92 3b c4 8e 75 6b b8 4a 9f 4a 3a 22 63 d3 a1 56 98 47 62 bc
> cd a6 8e d5 77 ...
> Apr 11 23:53:38 localhost pppd[31289]: Unsupported protocol 0xda7d received
> Apr 11 23:53:38 localhost pppd[31289]: sent [LCP ProtRej id=0xa da 7d
> 15 19 45 3c e0 ac 44 92 3b c4 8e 75 6b b8 4a 9f 4a 3a 22 63 d3 a1 56
> 98 47 62 bc cd a6 8e ...]
> Apr 11 23:53:40 localhost kernel: skb_under_panic: text:f8c62c0e
> len:291 put:1 head:ddc94800 data:ddc947ff tail:ddc94922 end:ddc94e00
> dev:<NULL>


It seems we fail to reserve enough headroom for the case
buf[0] == PPP_ALLSTATIONS and buf[1] != PPP_UI.

Can you try this patch please?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 841 bytes --]

diff --git a/drivers/net/ppp_async.c b/drivers/net/ppp_async.c
index 933e2f3..c68e37f 100644
--- a/drivers/net/ppp_async.c
+++ b/drivers/net/ppp_async.c
@@ -890,6 +890,8 @@ ppp_async_input(struct asyncppp *ap, const unsigned char *buf,
  				ap->rpkt = skb;
  			}
  			if (skb->len == 0) {
+				int headroom = 0;
+
  				/* Try to get the payload 4-byte aligned.
  				 * This should match the
  				 * PPP_ALLSTATIONS/PPP_UI/compressed tests in
@@ -897,7 +899,10 @@ ppp_async_input(struct asyncppp *ap, const unsigned char *buf,
  				 * enough chars here to test buf[1] and buf[2].
  				 */
 				if (buf[0] != PPP_ALLSTATIONS)
-					skb_reserve(skb, 2 + (buf[0] & 1));
+					headroom += 2;
+				if (buf[0] & 1)
+					headroom += 1;
+				skb_reserve(skb, headroom);
 			}
 			if (n > skb_tailroom(skb)) {
 				/* packet overflowed MRU */

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 12 Apr 2007 07:43:39 +0200

> Bartek wrote:
> > Hopefully, this time it my bug report should be ok :):
> > 
> > Apr 11 23:53:38 localhost pppd[31289]: rcvd [proto=0x7689] e1 cd 33 f6
> > fd f7 52 e6 58 c9 73 98 bc ff ad d5 b5 a3 e5 d9 1e 77 76 0a 1c 87 59
> > bf 44 cc ac 3b ...
> > Apr 11 23:53:38 localhost pppd[31289]: Unsupported protocol 0x7689 received
> > Apr 11 23:53:38 localhost pppd[31289]: sent [LCP ProtRej id=0x9 76 89
> > e1 cd 33 f6 fd f7 52 e6 58 c9 73 98 bc ff ad d5 b5 a3 e5 d9 1e 77 76
> > 0a 1c 87 59 bf 44 cc ...]
> > Apr 11 23:53:38 localhost pppd[31289]: rcvd [proto=0xda7d] 15 19 45 3c
> > e0 ac 44 92 3b c4 8e 75 6b b8 4a 9f 4a 3a 22 63 d3 a1 56 98 47 62 bc
> > cd a6 8e d5 77 ...
> > Apr 11 23:53:38 localhost pppd[31289]: Unsupported protocol 0xda7d received
> > Apr 11 23:53:38 localhost pppd[31289]: sent [LCP ProtRej id=0xa da 7d
> > 15 19 45 3c e0 ac 44 92 3b c4 8e 75 6b b8 4a 9f 4a 3a 22 63 d3 a1 56
> > 98 47 62 bc cd a6 8e ...]
> > Apr 11 23:53:40 localhost kernel: skb_under_panic: text:f8c62c0e
> > len:291 put:1 head:ddc94800 data:ddc947ff tail:ddc94922 end:ddc94e00
> > dev:<NULL>
> 
> 
> It seems we fail to reserve enough headroom for the case
> buf[0] == PPP_ALLSTATIONS and buf[1] != PPP_UI.
> 
> Can you try this patch please?

Any confirmation of this fix yet?

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
>
>> It seems we fail to reserve enough headroom for the case
>> buf[0] == PPP_ALLSTATIONS and buf[1] != PPP_UI.
>> 
>> Can you try this patch please?
> 
> Any confirmation of this fix yet?

FWIW the fix definitely looks correct (the bug has been there for
years at least) and it does match the crash dump.  In fact, there
is only a single skb_push (the only thing that can generate an under
panic) in ppp_async.c and this is the one that Patrick has fixed.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Paul Mackerras]

David Miller writes:

> > It seems we fail to reserve enough headroom for the case
> > buf[0] == PPP_ALLSTATIONS and buf[1] != PPP_UI.
> > 
> > Can you try this patch please?
> 
> Any confirmation of this fix yet?

Indeed, ppp_async doesn't handle that case correctly.

RFC 1662 says:

      The Control field is a single octet, which contains the binary
      sequence 00000011 (hexadecimal 0x03), the Unnumbered Information
      (UI) command with the Poll/Final (P/F) bit set to zero.

      The use of other Control field values may be defined at a later
      time, or by prior agreement.  Frames with unrecognized Control
      field values SHOULD be silently discarded.

In what situation were we getting the frames that cause the problem?

I didn't see the patch (the message that this is a reply to is the
first one that I have seen in this thread), so I can't comment on it.

Paul.

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[David Miller]

From: Paul Mackerras <paulus@samba.org>
Date: Sun, 15 Apr 2007 02:49:28 +1000

> I didn't see the patch (the message that this is a reply to is the
> first one that I have seen in this thread), so I can't comment on it.

Here is Patrick McHardy's patch:

diff --git a/drivers/net/ppp_async.c b/drivers/net/ppp_async.c
index 933e2f3..c68e37f 100644
--- a/drivers/net/ppp_async.c
+++ b/drivers/net/ppp_async.c
@@ -890,6 +890,8 @@ ppp_async_input(struct asyncppp *ap, const unsigned char *buf,
  				ap->rpkt = skb;
  			}
  			if (skb->len == 0) {
+				int headroom = 0;
+
  				/* Try to get the payload 4-byte aligned.
  				 * This should match the
  				 * PPP_ALLSTATIONS/PPP_UI/compressed tests in
@@ -897,7 +899,10 @@ ppp_async_input(struct asyncppp *ap, const unsigned char *buf,
  				 * enough chars here to test buf[1] and buf[2].
  				 */
 				if (buf[0] != PPP_ALLSTATIONS)
-					skb_reserve(skb, 2 + (buf[0] & 1));
+					headroom += 2;
+				if (buf[0] & 1)
+					headroom += 1;
+				skb_reserve(skb, headroom);
 			}
 			if (n > skb_tailroom(skb)) {
 				/* packet overflowed MRU */

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Patrick McHardy]

David Miller wrote:
> From: Paul Mackerras <paulus@samba.org>
> Date: Sun, 15 Apr 2007 02:49:28 +1000
> 
> 
>>I didn't see the patch (the message that this is a reply to is the
>>first one that I have seen in this thread), so I can't comment on it.
> 
> 
> Here is Patrick McHardy's patch:
> 
> [..]


I haven't heard back from Bartek yet. In case the patch is correct,
ppp_synctty seems to need the same fix.

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Paul Mackerras]

David Miller writes:

> Here is Patrick McHardy's patch:

So this doesn't change process_input_packet(), which treats the case
where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
0x03 (PPP_UI) as indicating a packet with a PPP protocol number of
0xff.  Arguably that's wrong since PPP protocol 0xff is reserved, and
the RFC does envision the possibility of receiving frames where the
control field has values other than 0x03.

Therefore I think this patch is probably better.  Could people try it
out and let me know if it fixes the problem?

Paul.

diff --git a/drivers/net/ppp_async.c b/drivers/net/ppp_async.c
index 933e2f3..caabbc4 100644
--- a/drivers/net/ppp_async.c
+++ b/drivers/net/ppp_async.c
@@ -802,9 +802,9 @@ process_input_packet(struct asyncppp *ap)
 
 	/* check for address/control and protocol compression */
 	p = skb->data;
-	if (p[0] == PPP_ALLSTATIONS && p[1] == PPP_UI) {
+	if (p[0] == PPP_ALLSTATIONS) {
 		/* chop off address/control */
-		if (skb->len < 3)
+		if (p[1] != PPP_UI || skb->len < 3)
 			goto err;
 		p = skb_pull(skb, 2);
 	}

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Paul Mackerras]

I wrote:

> So this doesn't change process_input_packet(), which treats the case
> where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
> 0x03 (PPP_UI) as indicating a packet with a PPP protocol number of

I meant "the second byte is NOT 0x03", of course.

Paul.

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[David Miller]

From: Paul Mackerras <paulus@samba.org>
Date: Sun, 15 Apr 2007 11:05:53 +1000

> I wrote:
> 
> > So this doesn't change process_input_packet(), which treats the case
> > where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
> > 0x03 (PPP_UI) as indicating a packet with a PPP protocol number of
> 
> I meant "the second byte is NOT 0x03", of course.

I've applied this patch, thanks Paul.

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Jarek Poplawski]

Hi,

I didn't analyse this bug report but probably it
is nearly connected with one of the bugs visible in
a log from this submit:

http://bugzilla.kernel.org/show_bug.cgi?id=8132

On 15-04-2007 02:50, Paul Mackerras wrote:
> David Miller writes:
> 
>> Here is Patrick McHardy's patch:
> 
> So this doesn't change process_input_packet(), which treats the case
> where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
> 0x03 (PPP_UI) as indicating a packet with a PPP protocol number of
> 0xff.  Arguably that's wrong since PPP protocol 0xff is reserved, and
> the RFC does envision the possibility of receiving frames where the
> control field has values other than 0x03.
> 
> Therefore I think this patch is probably better.  Could people try it
> out and let me know if it fixes the problem?
> 
> Paul.
> 
> diff --git a/drivers/net/ppp_async.c b/drivers/net/ppp_async.c
> index 933e2f3..caabbc4 100644
> --- a/drivers/net/ppp_async.c
> +++ b/drivers/net/ppp_async.c
> @@ -802,9 +802,9 @@ process_input_packet(struct asyncppp *ap)
>  
>  	/* check for address/control and protocol compression */
>  	p = skb->data;
> -	if (p[0] == PPP_ALLSTATIONS && p[1] == PPP_UI) {
> +	if (p[0] == PPP_ALLSTATIONS) {
>  		/* chop off address/control */
> -		if (skb->len < 3)
> +		if (p[1] != PPP_UI || skb->len < 3)
>  			goto err;
>  		p = skb_pull(skb, 2);
>  	}

Let's look farther:

>        proto = p[0];
>        if (proto & 1) {
>                /* protocol is compressed */
>                skb_push(skb, 1)[0] = 0;

BTW - about Patrick's patch:

skb_push seems to be dependent here on the 1-st char of
skb->data, if above (p[0] != PPP_ALLSTATIONS), but on the
3-rd char otherwise (after skb_pull). But, Patrick's patch
reserves the place for this, looking always at 1-st char
(buf[0]) independently of PPP_ALLSTATIONS char presence,
or otherwise - always treating this char as protocol char.
It looks safe because of PPP_ALLSTATION current value,
but isn't too understandable.

On the other hand, without any reservation in the
ppp_async_input for the (buf[0] == PPP_ALLSTATIONS) case,
probably 4-byte alignement isn't achieved as planned. 

>        } else {
>                if (skb->len < 2)
>                        goto err;
>                proto = (proto << 8) + p[1];
>                if (proto == PPP_LCP)
>                        async_lcp_peek(ap, p, skb->len, 1);
>        }
>
>        /* queue the frame to be processed */
>        skb->cb[0] = ap->state;
>        skb_queue_tail(&ap->rqueue, skb);
>        ap->rpkt = NULL;
>        ap->state = 0;
>        return;
>
> err:
>        /* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
>        ap->state = SC_PREV_ERROR;
>        if (skb) {
>                /* make skb appear as freshly allocated */

Probably this isn't always true and here the problem
started...

>                skb_trim(skb, 0);
>                skb_reserve(skb, - skb_headroom(skb));

Isn't here lost e.g. NET_SKB_PAD probably reserved by
dev_alloc_skb?

On the other hand - this kind of pad can very good hide
similar reservation problems in many other places - maybe
it should be omitted or somehow counted in WARNs when some
debugging options are active?

Regards,
Jarek P.

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Herbert Xu]

Hi Paul:

Paul Mackerras <paulus@samba.org> wrote:
> 
> So this doesn't change process_input_packet(), which treats the case
> where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
> 0x03 (PPP_UI) as indicating a packet with a PPP protocol number of
> 0xff.  Arguably that's wrong since PPP protocol 0xff is reserved, and
> the RFC does envision the possibility of receiving frames where the
> control field has values other than 0x03.

Your fix is probably needed too.  However, I think the issue that Patrick
was trying to fix is the case where p[0] != PPP_ALLSTATIONS and therefore
we'd still have a problem there.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Bartek]

> Your fix is probably needed too.  However, I think the issue that Patrick
> was trying to fix is the case where p[0] != PPP_ALLSTATIONS and therefore
> we'd still have a problem there.

I tested Paul's patch for last few days and I think everything seems
ok. The system is stable.

Regards
Bartek

Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6

[Herbert Xu]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> Paul Mackerras <paulus@samba.org> wrote:
>> 
>> So this doesn't change process_input_packet(), which treats the case
>> where the first byte is 0xff (PPP_ALLSTATIONS) but the second byte is
>> 0x03 (PPP_UI) as indicating a packet with a PPP protocol number of
>> 0xff.  Arguably that's wrong since PPP protocol 0xff is reserved, and
>> the RFC does envision the possibility of receiving frames where the
>> control field has values other than 0x03.
> 
> Your fix is probably needed too.  However, I think the issue that Patrick
> was trying to fix is the case where p[0] != PPP_ALLSTATIONS and therefore
> we'd still have a problem there.

Nevermind, I mixed up != with == so your patch is all we need.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt