Re: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
I wrote :
> I just noticed something at the end of process_input_packet :
> In the normal case, skb is given to the next stage and ap->rpkt is reset,
> but in the error case, skb is kept, ap->rpkt is not reset, so we keep
> the skb with skb->data aligned for one message and we put another one
> into it :)
>
> Could that not be the culprit ?
Based on my previous observation, here is a revised patch, that replaces
the previous one.
This patch avoids ppp-generated kernel crashes on machines where
unaligned accesses are forbidden, by fixing ppp alignment setting
for reused skb's.
Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]>
--- drivers/net/ppp_async.c 2004/05/07 08:38:32 1.1.1.1
+++ drivers/net/ppp_async.c 2005/08/11 11:21:33
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#define PPP_VERSION"2.4.2"
@@ -846,7 +847,11 @@ process_input_packet(struct asyncppp *ap
/* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
ap->state = SC_PREV_ERROR;
if (skb)
+ {
+ /* make skb appear as freshly allocated */
skb_trim(skb, 0);
+ skb_reserve(skb, - skb_headroom(skb));
+ }
}
/* called when the tty driver has data for us. */
@@ -897,10 +902,18 @@ ppp_async_input(struct asyncppp *ap, con
skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2);
if (skb == 0)
goto nomem;
+ ap->rpkt = skb;
+ }
+ if (skb->len == 0) {
/* Try to get the payload 4-byte aligned */
+ /* This should match the
+ ** PPP_ALLSTATIONS/PPP_UI/compressed tests
+ ** in process_input_packet,
+ ** but we do not have enough chars here to
+ ** test buf[1] and buf[2].
+ */
if (buf[0] != PPP_ALLSTATIONS)
skb_reserve(skb, 2 + (buf[0] & 1));
- ap->rpkt = skb;
}
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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote : > Philippe De Muyter writes: > > > Actually, that's probably the case I had, but my fix gets the ip adresses > > 4byte aligned in my case : I had verified the address of the saddr field, > > and I needed to shift the buffer by 3, not 1, to get it 4byte aligned. > > Please outline the code flow that leads to that situation. AFAICS we > would only need to shift the buffer by 3 if we had a compressed > (1-byte) protocol number at the original skb->data, implying that the > protocol byte was first. But if the protocol byte was first, then > this code: > > if (buf[0] != PPP_ALLSTATIONS) > skb_reserve(skb, 2 + (buf[0] & 1)); > > at line 893 should have moved skb->data up by 3 bytes already, since a > 1-byte protocol number is always odd. > > If that is not the case then there is something else going on beyond > the data getting misaligned, and I would like to know what it is. I just noticed something at the end of process_input_packet : /* queue the frame to be processed */ skb->cb[0] = ap->state; skb_queue_tail(&ap->rqueue, skb); ap->rpkt = 0; ap->state = 0; return; err: /* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */ ap->state = SC_PREV_ERROR; if (skb) skb_trim(skb, 0); } In the normal case, skb is given to the next stage and ap->rpkt is reset, but in the error case, skb is kept, ap->rpkt is not reset, so we keep the skb with skb->data aligned for one message and we put another one into it :) Could that not be the culprit ? Philippe > > 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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Philippe De Muyter writes: > Actually, that's probably the case I had, but my fix gets the ip adresses > 4byte aligned in my case : I had verified the address of the saddr field, > and I needed to shift the buffer by 3, not 1, to get it 4byte aligned. Please outline the code flow that leads to that situation. AFAICS we would only need to shift the buffer by 3 if we had a compressed (1-byte) protocol number at the original skb->data, implying that the protocol byte was first. But if the protocol byte was first, then this code: if (buf[0] != PPP_ALLSTATIONS) skb_reserve(skb, 2 + (buf[0] & 1)); at line 893 should have moved skb->data up by 3 bytes already, since a 1-byte protocol number is always odd. If that is not the case then there is something else going on beyond the data getting misaligned, and I would like to know what it is. 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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote :
> Philippe De Muyter writes:
>
> > > This patch seems a bit strange and/or incomplete. Are we trying to
> > > get 2-byte alignment or 4-byte alignment of the payload? It seems
> >
> > Actually, we try to get a 4n+2 alignment for skb->data, to get the
> > ip-addresses
> > field 4bytes aligned.
> > I think the only thing wrong is the old comment that said :
> > /* Try to get the payload 4-byte aligned */
> > and that I did not change.
>
> Yes, the payload is the part after the protocol field, so the comment
> is still correct.
>
> > > that if the protocol field is uncompressed, we don't do anything to
> > > the alignment, but if it is compressed, we do this:
> > >
> > > > /* protocol is compressed */
> > > > - skb_push(skb, 1)[0] = 0;
> > > > + if ((unsigned long)skb->data & 1)
> > > > + skb_push(skb, 1)[0] = 0;
> > > > + else { /* Ditto, but realign the payload to 4-byte
> > > > boundary */
> > > > + short len = skb->len;
> > > > +
> > > > + skb_put(skb, 3);
> > > > + memmove(skb->data + 3, skb->data, len);
> > > > + skb_pull(skb, 2)[0] = 0;
> > >
> > > I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> > > we are really trying to achieve 4-byte alignment. In fact, if the
> > > skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> > > with, this will end up with the payload starting at the original
> > > skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
> > >
> > > Can we assume that dev_alloc_skb will give us a 4-byte aligned
> > > skb->data? If we can then I suggest we change 3 to 1 in the skb_put
> >
> > Are you not forgetting that the alignment of skb->data is changed (by the
> > existing code ! ) :
> > if (buf[0] != PPP_ALLSTATIONS)
> > skb_reserve(skb, 2 + (buf[0] & 1));
>
> No, I'm not forgetting. If we assume that skb->data starts out 4-byte
> aligned, then the only case in which we will have
>
> ((unsigned long)skb->data & 1) == 0
>
> is if we have protocol field compression (and a compressible protocol
> number, i.e. less than 0x100) but not address/control compression
> (which would be a weird combination, but legal). In that case, with
> your patch we will move the protocol byte to the original skb->data+5
> and have the payload at +6.
Actually, that's probably the case I had, but my fix gets the ip adresses
4byte aligned in my case : I had verified the address of the saddr field,
and I needed to shift the buffer by 3, not 1, to get it 4byte aligned.
>
> If there is any possibility that skb->data is not 4-byte aligned when
No, that's not the problem I had. skb->data was always 4-byte aligned
at allocation time.
> the skb is first allocated, I think that we should do something like
>
> if ((unsigned long)skb->data & 3)
> skb_reserve(skb, 4 - ((unsigned long)skb->data & 3));
>
> immediately after allocating it, and then just memmove the stuff up
> one byte (rather than 3) if it isn't aligned as we want.
>
> Paul.
>
Philippe
-
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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Philippe De Muyter writes:
> > This patch seems a bit strange and/or incomplete. Are we trying to
> > get 2-byte alignment or 4-byte alignment of the payload? It seems
>
> Actually, we try to get a 4n+2 alignment for skb->data, to get the
> ip-addresses
> field 4bytes aligned.
> I think the only thing wrong is the old comment that said :
> /* Try to get the payload 4-byte aligned */
> and that I did not change.
Yes, the payload is the part after the protocol field, so the comment
is still correct.
> > that if the protocol field is uncompressed, we don't do anything to
> > the alignment, but if it is compressed, we do this:
> >
> > > /* protocol is compressed */
> > > - skb_push(skb, 1)[0] = 0;
> > > + if ((unsigned long)skb->data & 1)
> > > + skb_push(skb, 1)[0] = 0;
> > > + else { /* Ditto, but realign the payload to 4-byte boundary */
> > > + short len = skb->len;
> > > +
> > > + skb_put(skb, 3);
> > > + memmove(skb->data + 3, skb->data, len);
> > > + skb_pull(skb, 2)[0] = 0;
> >
> > I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> > we are really trying to achieve 4-byte alignment. In fact, if the
> > skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> > with, this will end up with the payload starting at the original
> > skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
> >
> > Can we assume that dev_alloc_skb will give us a 4-byte aligned
> > skb->data? If we can then I suggest we change 3 to 1 in the skb_put
>
> Are you not forgetting that the alignment of skb->data is changed (by the
> existing code ! ) :
> if (buf[0] != PPP_ALLSTATIONS)
> skb_reserve(skb, 2 + (buf[0] & 1));
No, I'm not forgetting. If we assume that skb->data starts out 4-byte
aligned, then the only case in which we will have
((unsigned long)skb->data & 1) == 0
is if we have protocol field compression (and a compressible protocol
number, i.e. less than 0x100) but not address/control compression
(which would be a weird combination, but legal). In that case, with
your patch we will move the protocol byte to the original skb->data+5
and have the payload at +6.
If there is any possibility that skb->data is not 4-byte aligned when
the skb is first allocated, I think that we should do something like
if ((unsigned long)skb->data & 3)
skb_reserve(skb, 4 - ((unsigned long)skb->data & 3));
immediately after allocating it, and then just memmove the stuff up
one byte (rather than 3) if it isn't aligned as we want.
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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote :
> Jeff Garzik writes:
>
> > From: "Philippe De Muyter" <[EMAIL PROTECTED]>
> >
> > Avoid ppp-generated kernel crashes on machines where unaligned accesses are
> > forbidden (ie: 68000-based CPUs)
>
> This patch seems a bit strange and/or incomplete. Are we trying to
> get 2-byte alignment or 4-byte alignment of the payload? It seems
Actually, we try to get a 4n+2 alignment for skb->data, to get the ip-addresses
field 4bytes aligned.
I think the only thing wrong is the old comment that said :
/* Try to get the payload 4-byte aligned */
and that I did not change.
> that if the protocol field is uncompressed, we don't do anything to
> the alignment, but if it is compressed, we do this:
>
> > /* protocol is compressed */
> > - skb_push(skb, 1)[0] = 0;
> > + if ((unsigned long)skb->data & 1)
> > + skb_push(skb, 1)[0] = 0;
> > + else { /* Ditto, but realign the payload to 4-byte boundary */
> > + short len = skb->len;
> > +
> > + skb_put(skb, 3);
> > + memmove(skb->data + 3, skb->data, len);
> > + skb_pull(skb, 2)[0] = 0;
>
> I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> we are really trying to achieve 4-byte alignment. In fact, if the
> skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> with, this will end up with the payload starting at the original
> skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
>
> Can we assume that dev_alloc_skb will give us a 4-byte aligned
> skb->data? If we can then I suggest we change 3 to 1 in the skb_put
Are you not forgetting that the alignment of skb->data is changed (by the
existing code ! ) :
if (buf[0] != PPP_ALLSTATIONS)
skb_reserve(skb, 2 + (buf[0] & 1));
> and memmove above, and get rid of the if (since its condition will
> always be false).
>
> Paul.
>
Philippe
-
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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote: > Can we assume that dev_alloc_skb will give us a 4-byte aligned > skb->data? If we can then I suggest we change 3 to 1 in the skb_put > and memmove above, and get rid of the if (since its condition will > always be false). The usual kmalloc alignment rules hold for skb->head. skb->data may be of any alignment, depending on where it is used. I don't know enough about ppp to tell how it will look in this case. Regards Patrick - 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: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Jeff Garzik writes:
> From: "Philippe De Muyter" <[EMAIL PROTECTED]>
>
> Avoid ppp-generated kernel crashes on machines where unaligned accesses are
> forbidden (ie: 68000-based CPUs)
This patch seems a bit strange and/or incomplete. Are we trying to
get 2-byte alignment or 4-byte alignment of the payload? It seems
that if the protocol field is uncompressed, we don't do anything to
the alignment, but if it is compressed, we do this:
> /* protocol is compressed */
> - skb_push(skb, 1)[0] = 0;
> + if ((unsigned long)skb->data & 1)
> + skb_push(skb, 1)[0] = 0;
> + else { /* Ditto, but realign the payload to 4-byte boundary */
> + short len = skb->len;
> +
> + skb_put(skb, 3);
> + memmove(skb->data + 3, skb->data, len);
> + skb_pull(skb, 2)[0] = 0;
I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
we are really trying to achieve 4-byte alignment. In fact, if the
skb->data that we get from dev_alloc_skb is 4-byte aligned to start
with, this will end up with the payload starting at the original
skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
Can we assume that dev_alloc_skb will give us a 4-byte aligned
skb->data? If we can then I suggest we change 3 to 1 in the skb_put
and memmove above, and get rid of the if (since its condition will
always be false).
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
