Bug 27767
| Summary: | PPPoE makes 32-bit assumptions and breaks on an Alpha | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Sudish Joseph <sudish> | ||||
| Component: | kern | Assignee: | Brian Somers <brian> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | 4.3-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Sudish Joseph
2001-05-30 10:50:01 UTC
Responsible Changed From-To: freebsd-bugs->brian I'll look at this Hi, I think the attached will fix it properly -- but I haven't got an alpha to test with, so can someone do the honours for me ? Ta. > can you send us a patch that works for you? > we can make it #ifdef __Alpha__ or something. > > can you ocnfirm that the outgoing packet has a tag-lenth of '8' > and that teh return tag has a length of '4'? > (maybe 9 and 5) > > sounds like a brain-dead router at the other end.. > > > On Wed, 1 Aug 2001, Thomas Pornin wrote: > > > Hello, > > > > I recently connected my FreeBSD/Alpha (4.3-RELEASE) to an ADSL link > > using an Alcatel Speed Touch Home modem. As is, it was not working; > > after some digging, I found that there is a bug either in the ng_pppoe > > support, or in the modem. > > > > The problem is in the pppoe_finduniq() function. In order to identify > > sessions, the PPPoE code sends a tag with the first packet it sends to > > the modem; this tag is in fact a 64-bit pointer to some data structure > > in kernel space. When a packet of type PADO_CODE or PADS_CODE is > > received, the tag is compared with known pointers. > > > > However, only 32 bits are present in the return tag. So, if the original > > pointer is 0xfffffc00003b3d00, the tag contains only 0x003b3d00, which > > are the first four bytes of data (in little-endian representation). If > > I modify the pppoe_finduniq() function to accept matches on these four > > bytes, the connection is established, and remains fully functionnal > > afterwards. > > > > Some details: The Alpha is little-endian, but the data in the packets is > > big-endian. If the original pointer is 0xfffffc00003b3d00, the rebuilt > > tag from the response packet is actually 0x003b3d0000000000. I do not > > know if the 8-bytes tag is sent correctly, or if the modem is buggy, or > > whatever. > > > > Machine configuration: > > AXPpci33 at 166 MHz, 32 MB ram > > ethernet PCI adapter RealTek 8029 10baseT (ed0) > > modem Alcatel Speed Touch Home (ethernet) > > FreeBSD 4.3-RELEASE (with a small patch to enable ed0) > > > > Feel free to ask for any detail. > > > > > > --Thomas Pornin -- Brian <brian@freebsd-services.com> <brian@Awfulhak.org> http://www.freebsd-services.com/ <brian@[uk.]FreeBSD.org> Don't _EVER_ lose your sense of humour ! <brian@[uk.]OpenBSD.org> Index: ng_pppoe.c =================================================================== RCS file: /home/ncvs/src/sys/netgraph/ng_pppoe.c,v retrieving revision 1.45 diff -u -r1.45 ng_pppoe.c --- ng_pppoe.c 2001/07/25 03:34:07 1.45 +++ ng_pppoe.c 2001/08/01 22:40:15 @@ -868,7 +868,7 @@ struct { struct pppoe_tag hdr; union uniq data; - } uniqtag; + } __attribute ((packed)) uniqtag; /* * kick the state machine into starting up @@ -910,7 +910,7 @@ struct { struct pppoe_tag hdr; union uniq data; - } uniqtag; + } __attribute ((packed)) uniqtag; negp neg = NULL; struct mbuf *m; DUH!
(slaps forhead with palm "obvious when shown")
can you try this thomas?
On Wed, 1 Aug 2001, Brian Somers wrote:
> Hi,
>
> I think the attached will fix it properly -- but I haven't got an
> alpha to test with, so can someone do the honours for me ?
>
> Ta.
>
> > can you send us a patch that works for you?
> > we can make it #ifdef __Alpha__ or something.
> >
> > can you ocnfirm that the outgoing packet has a tag-lenth of '8'
> > and that teh return tag has a length of '4'?
> > (maybe 9 and 5)
> >
> > sounds like a brain-dead router at the other end..
> >
> >
> > On Wed, 1 Aug 2001, Thomas Pornin wrote:
> >
> > > Hello,
> > >
> > > I recently connected my FreeBSD/Alpha (4.3-RELEASE) to an ADSL link
> > > using an Alcatel Speed Touch Home modem. As is, it was not working;
> > > after some digging, I found that there is a bug either in the ng_pppoe
> > > support, or in the modem.
> > >
> > > The problem is in the pppoe_finduniq() function. In order to identify
> > > sessions, the PPPoE code sends a tag with the first packet it sends to
> > > the modem; this tag is in fact a 64-bit pointer to some data structure
> > > in kernel space. When a packet of type PADO_CODE or PADS_CODE is
> > > received, the tag is compared with known pointers.
> > >
> > > However, only 32 bits are present in the return tag. So, if the original
> > > pointer is 0xfffffc00003b3d00, the tag contains only 0x003b3d00, which
> > > are the first four bytes of data (in little-endian representation). If
> > > I modify the pppoe_finduniq() function to accept matches on these four
> > > bytes, the connection is established, and remains fully functionnal
> > > afterwards.
> > >
> > > Some details: The Alpha is little-endian, but the data in the packets is
> > > big-endian. If the original pointer is 0xfffffc00003b3d00, the rebuilt
> > > tag from the response packet is actually 0x003b3d0000000000. I do not
> > > know if the 8-bytes tag is sent correctly, or if the modem is buggy, or
> > > whatever.
> > >
> > > Machine configuration:
> > > AXPpci33 at 166 MHz, 32 MB ram
> > > ethernet PCI adapter RealTek 8029 10baseT (ed0)
> > > modem Alcatel Speed Touch Home (ethernet)
> > > FreeBSD 4.3-RELEASE (with a small patch to enable ed0)
> > >
> > > Feel free to ask for any detail.
> > >
> > >
> > > --Thomas Pornin
>
> --
> Brian <brian@freebsd-services.com> <brian@Awfulhak.org>
> http://www.freebsd-services.com/ <brian@[uk.]FreeBSD.org>
> Don't _EVER_ lose your sense of humour ! <brian@[uk.]OpenBSD.org>
>
> Index: ng_pppoe.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/netgraph/ng_pppoe.c,v
> retrieving revision 1.45
> diff -u -r1.45 ng_pppoe.c
> --- ng_pppoe.c 2001/07/25 03:34:07 1.45
> +++ ng_pppoe.c 2001/08/01 22:40:15
> @@ -868,7 +868,7 @@
> struct {
> struct pppoe_tag hdr;
> union uniq data;
> - } uniqtag;
> + } __attribute ((packed)) uniqtag;
>
> /*
> * kick the state machine into starting up
> @@ -910,7 +910,7 @@
> struct {
> struct pppoe_tag hdr;
> union uniq data;
> - } uniqtag;
> + } __attribute ((packed)) uniqtag;
> negp neg = NULL;
> struct mbuf *m;
>
>
>
>
On Wed, Aug 01, 2001 at 06:11:11PM -0700, Julian Elischer wrote:
> can you try this thomas?
I tried. It works like a charm. Thanks !
--Thomas
State Changed From-To: open->closed Fixed in -current. I'll MFC in 1 week On Wed, 1 Aug 2001, Julian Elischer wrote: > DUH! > (slaps forhead with palm "obvious when shown") > On Wed, 1 Aug 2001, Brian Somers wrote: > > Index: ng_pppoe.c > > =================================================================== > > RCS file: /home/ncvs/src/sys/netgraph/ng_pppoe.c,v > > retrieving revision 1.45 > > diff -u -r1.45 ng_pppoe.c > > --- ng_pppoe.c 2001/07/25 03:34:07 1.45 > > +++ ng_pppoe.c 2001/08/01 22:40:15 > > @@ -868,7 +868,7 @@ > > struct { > > struct pppoe_tag hdr; > > union uniq data; > > - } uniqtag; > > + } __attribute ((packed)) uniqtag; > > > > /* > > * kick the state machine into starting up > > @@ -910,7 +910,7 @@ > > struct { > > struct pppoe_tag hdr; > > union uniq data; > > - } uniqtag; > > + } __attribute ((packed)) uniqtag; > > negp neg = NULL; > > struct mbuf *m; Obviously a syntax error. Bruce Why my arp table is wrong???? the windows machines and my freebsd box are with crazy arp table!!! my freebsd box sad: arp 10.0.0.1 moved from xxxxx to xxxxx and don't ping anything... my windows machines too. but windows don't tell me anything!!!! i have one server dhcp (FreeBSD) and this machine run NAT for my dhcp clients. another problem is that two or more machines ping one internet host, the reply go to just one. thanks!!!! help... sorry by the english. care to explain?
On Fri, 3 Aug 2001, Bruce Evans wrote:
> On Wed, 1 Aug 2001, Julian Elischer wrote:
>
> > DUH!
> > (slaps forhead with palm "obvious when shown")
>
> > On Wed, 1 Aug 2001, Brian Somers wrote:
> > > Index: ng_pppoe.c
> > > ===================================================================
> > > RCS file: /home/ncvs/src/sys/netgraph/ng_pppoe.c,v
> > > retrieving revision 1.45
> > > diff -u -r1.45 ng_pppoe.c
> > > --- ng_pppoe.c 2001/07/25 03:34:07 1.45
> > > +++ ng_pppoe.c 2001/08/01 22:40:15
> > > @@ -868,7 +868,7 @@
> > > struct {
> > > struct pppoe_tag hdr;
> > > union uniq data;
> > > - } uniqtag;
> > > + } __attribute ((packed)) uniqtag;
> > >
> > > /*
> > > * kick the state machine into starting up
> > > @@ -910,7 +910,7 @@
> > > struct {
> > > struct pppoe_tag hdr;
> > > union uniq data;
> > > - } uniqtag;
> > > + } __attribute ((packed)) uniqtag;
> > > negp neg = NULL;
> > > struct mbuf *m;
>
> Obviously a syntax error.
>
> Bruce
>
>
> To Unsubscribe: send mail to majordomo@FreeBSD.org
> with "unsubscribe freebsd-net" in the body of the message
>
On Thu, 2 Aug 2001, Julian Elischer wrote: > care to explain? Please trim quotes and reply after the quoted section[s]. > On Fri, 3 Aug 2001, Bruce Evans wrote: > > > On Wed, 1 Aug 2001, Brian Somers wrote: > > > > @@ -910,7 +910,7 @@ > > > > struct { > > > > struct pppoe_tag hdr; > > > > union uniq data; > > > > - } uniqtag; > > > > + } __attribute ((packed)) uniqtag; > > > > negp neg = NULL; > > > > struct mbuf *m; > > > > Obviously a syntax error. `__attribute ((packed))' is a syntax error in Standard C. Dependencies on compiler features are supposed to be centralized in places like <sys/cdefs.h>. Many invocations of __attribute__(()) are placed there. This is not possible for the `packed' attribute because this attribute has non-cosmetic effects. Structs must be carefully laid out to give some chance of them not having any internal padding for any compiler. This sometimes involves using struct members with an unnatural type. <fs/msdosfs/bpb.h> has many examples. Here I don't see how packing the struct helps on alphas. Doesn't it cause traps by misaligning uniqtag.data.pointer? I think you need something like: struct { struct pppoe_tag hdr; char data[sizeof(void *)]; } uniqtag; void *pointer; ... pointer = sp; memcpy(uniqtag.data, &pointer, sizeof(uniqtag.data)); Bruce > `__attribute ((packed))' is a syntax error in Standard C. > > Dependencies on compiler features are supposed to be centralized in > places like <sys/cdefs.h>. Many invocations of __attribute__(()) are > placed there. This is not possible for the `packed' attribute because > this attribute has non-cosmetic effects. Structs must be carefully > laid out to give some chance of them not having any internal padding > for any compiler. This sometimes involves using struct members with > an unnatural type. <fs/msdosfs/bpb.h> has many examples. > > Here I don't see how packing the struct helps on alphas. Doesn't it > cause traps by misaligning uniqtag.data.pointer? I think you need > something like: It doesn't cause a trap as the compiler is (now) smart enough to handle it properly. Note where we have the alignment problem here: beast:~ $ cat x.c #include <stdio.h> struct s { short x, y; }; union u { char c[sizeof(void*)]; void *v; }; struct x { struct s s; union u u; } __attribute ((packed)); int main() { struct x x; char c[] = "hello"; int i; x.u.v = c; /* This is ok despite it not being aligned */ i = (char *)&x.u.v - (char *)&x; printf("Got \"%s\", offset %d\n", (char *)x.u.v, i); *(void **)((char *)&x + 4) = c; /* This is not ok */ printf("Without knowledge of packed... \"%s\"\n", (char *)x.u.v); return 0; } beast:~ $ cc -o x x.c beast:~ $ ./x Got "hello", offset 4 pid 30842 (x): unaligned access: va=0x11ffba64 pc=0x120000894 ra=0x120000884 op=stq Without knowledge of packed... "hello" > struct { > struct pppoe_tag hdr; > char data[sizeof(void *)]; > } uniqtag; > void *pointer; > ... > pointer = sp; > memcpy(uniqtag.data, &pointer, sizeof(uniqtag.data)); That's probably better. I went with the __attribute((packed)) stuff because it was already done that way in the rest of the code. In this specific case however, it may be best to just send the variable plus the alignment padding as the tag: struct { struct pppoe_tag hdr; union uniq data; } uniqtag; ...... uniqtag.hdr.tag_type = PTT_HOST_UNIQ; uniqtag.hdr.tag_len = htons((u_int16_t)(sizeof(uniqtag) - sizeof(uniqtag.hdr)); uniqtag.data.pointer = sp; But I'm not convinced it gains anything given the other uses of __attribute((packed)) in this code. > Bruce -- Brian <brian@freebsd-services.com> <brian@Awfulhak.org> http://www.freebsd-services.com/ <brian@[uk.]FreeBSD.org> Don't _EVER_ lose your sense of humour ! <brian@[uk.]OpenBSD.org> On Fri, 3 Aug 2001, Brian Somers wrote:
>
> That's probably better. I went with the __attribute((packed)) stuff
> because it was already done that way in the rest of the code.
>
> In this specific case however, it may be best to just send the variable
> plus the alignment padding as the tag:
>
> struct {
> struct pppoe_tag hdr;
> union uniq data;
> } uniqtag;
>
> ......
> uniqtag.hdr.tag_type = PTT_HOST_UNIQ;
> uniqtag.hdr.tag_len = htons((u_int16_t)(sizeof(uniqtag) - sizeof(uniqtag.hdr));
> uniqtag.data.pointer = sp;
urk
I prefer bruce's method :-) but if the rest use
packed.. we can probably use it here to be consistant..
On Fri, 3 Aug 2001, Brian Somers wrote: > > [bde wrote] > > Here I don't see how packing the struct helps on alphas. Doesn't it > > cause traps by misaligning uniqtag.data.pointer? I think you need > > something like: > > It doesn't cause a trap as the compiler is (now) smart enough to > handle it properly. Note where we have the alignment problem here: I guess it knows how to handle this in packed structs and not in many other cases. > > struct { > > struct pppoe_tag hdr; > > char data[sizeof(void *)]; > > } uniqtag; > > void *pointer; > > ... > > pointer = sp; > > memcpy(uniqtag.data, &pointer, sizeof(uniqtag.data)); > > That's probably better. I went with the __attribute((packed)) stuff > because it was already done that way in the rest of the code. I think the packed attributes in ng_pppoe.h are no-ops on most machines. > In this specific case however, it may be best to just send the variable > plus the alignment padding as the tag: > > struct { > struct pppoe_tag hdr; > union uniq data; > } uniqtag; > > ...... > uniqtag.hdr.tag_type = PTT_HOST_UNIQ; > uniqtag.hdr.tag_len = htons((u_int16_t)(sizeof(uniqtag) - sizeof(uniqtag.hdr)); > uniqtag.data.pointer = sp; You might also have to zero everything to ensure that there is no stack garbage in the message. Bruce |
