Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
From: Paul Mackerras <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
Herbert Xu <[EMAIL PROTECTED]> wrote:
>
> Paul Mackerras <[EMAIL PROTECTED]> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
Hi Paul:
Paul Mackerras <[EMAIL PROTECTED]> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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);
}
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
David Miller wrote: > From: Paul Mackerras <[EMAIL PROTECTED]> > 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
From: Paul Mackerras <[EMAIL PROTECTED]>
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 */
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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 0011 (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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
David Miller <[EMAIL PROTECTED]> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
From: Patrick McHardy <[EMAIL PROTECTED]> 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: > > > 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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: kernel BUG at net/core/skbuff.c in linux-2.6.21-rc6
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:
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?
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 */
