IPX routing in 5.4-RC3 doesn't work. See also: kern/74105 Some more structures need to be declared as "packed" for IPXrouted(8) and kernel routines that use "struct sockaddr_ipx" so that routes are created correctly. Fix: Patches against 5.4-RC3 How-To-Repeat: Find a busy IPX network. Run "IPXrouted -q -t". Notice misaligned hostnames, invalid IPX network addresses, negative ticks and extreme metric. Run "netstat -rnf ipx". Notice how all "Destinations" are "default" rather than "network.*" Run "ncplist s". Wait for output (it'll likely timeout).
On Fri, 22 Apr 2005, Keith White wrote: >> Description: > > IPX routing in 5.4-RC3 doesn't work. > > See also: kern/74105 > > Some more structures need to be declared as "packed" for IPXrouted(8) > and kernel routines that use "struct sockaddr_ipx" so that > routes are created correctly. Fewer structures need to be declared as packed. "packed" is unportable and doesn't even work. > Patches against 5.4-RC3 > > --- usr/src/usr.sbin/IPXrouted/protocol.h Fri Aug 27 21:15:03 1999 > +++ /usr/src/usr.sbin/IPXrouted/protocol.h Fri Apr 22 11:30:40 2005 > @@ -49,12 +49,12 @@ > union ipx_net rip_dst; /* destination net */ The bug is that all (?) ipx structs were naturally packed, but this was broken by adding a "u_int u_net;" to union ipx_net. > ... > --- usr/src/sys/netipx/ipx.h Mon Jan 31 18:26:42 2005 > +++ /usr/src/sys/netipx/ipx.h Fri Apr 22 14:13:51 2005 > @@ -130,7 +130,7 @@ > u_char sipx_family; > struct ipx_addr sipx_addr; > char sipx_zero[2]; > -}; > +} __packed; > #define sipx_port sipx_addr.x_port > #define sipx_network sipx_addr.x_net.u_net > #define sipx_node sipx_addr.x_host.c_host Declaring this struct (struct sockaddr_ipx) as packed turns u_net into complete nonsense and shows why "packed" should never be used. struct sockaddr_ipx begins with 2 u_chars and struct ipx_addr begins with union ipx_net, so if the struct sockaddr_ipx is aligned to a 32-bit or stricter boundary, then if the struct is also packed then u_net is never properly aligned so it never works properly in the only place that it is used (in sipx_network. It works on machines that don't trap for misaligned accesses). Using "packed" causes this problem in general. Accesses to struct members except 8-bit ones tends to break unless "packed" had no effect. ipx.h has always had union ipx_net_u that was apparently intended to either handled alignment stuff or to make struct ipx_net's accessible via an integral type like u_net does: %%% union ipx_net { u_char c_net[4]; u_short s_net[2]; u_int u_net; }; union ipx_net_u { union ipx_net net_e; u_long long_e; }; %%% but neither ipx_net_u nor long_e seems to be actually used, so it is hard to tell whether they would work. They were cloned from similarly unused code in netns. long_e probably needs to be 32-bit (type n_long in netinet) to actually work. Any use would face the same alignment problems as does u_net, but putting the integral type in a separate stuct at least keeps it out of all the structs that use ipx_net and thus prevents alignment poisoning. Bruce
On Sun, 24 Apr 2005, Bruce Evans wrote: > > The bug is that all (?) ipx structs were naturally packed, but this > was broken by adding a "u_int u_net;" to union ipx_net. > ... Indeed. If I try a fresh build of 5.4-RC3 with "union ipx_net" defined without "u_int u_net", both "IPXrouted" and "netstat -rnf ipx" work as expected. No additional "packed" attributes are required. Does the change to "src/sys/netipx/ipx.h" need to be reverted (again)? I don't use the mars_nwe port so don't know the ramifications there. Thanks for the hint! ...keith The following patch against 5.4-RC3 "works for me": --- usr/src/sys/netipx/ipx.h Mon Jan 31 18:26:42 2005 +++ /usr/src/sys/netipx/ipx.h Sat Apr 23 11:38:37 2005 @@ -108,7 +108,7 @@ union ipx_net { u_char c_net[4]; u_short s_net[2]; - u_int u_net; +/* u_int u_net; */ }; union ipx_net_u {
Responsible Changed From-To: freebsd-bugs->rwatson rwatson worked on this code
On Sat, 23 Apr 2005, Keith White wrote: > On Sun, 24 Apr 2005, Bruce Evans wrote: >> >> The bug is that all (?) ipx structs were naturally packed, but this >> was broken by adding a "u_int u_net;" to union ipx_net. >> ... > > Indeed. If I try a fresh build of 5.4-RC3 with "union ipx_net" > defined without "u_int u_net", both "IPXrouted" and > "netstat -rnf ipx" work as expected. No additional "packed" > attributes are required. Does the change to "src/sys/netipx/ipx.h" > need to be reverted (again)? I don't use the mars_nwe port so don't > know the ramifications there. I think it is a very wrong way to go, so it should be reverted. I think declaring only u_net as __packed would work. I said that the new u_net field becomes more bogus if all the structs containing it are packed because it becomes misaligned. That is incorrect what actually happens (appart from the code being source-code uncompilable and binary-incompatible with compilers that don't support "packed") is that the compiler has to assume that everything is misaligned so it has to load access everything a byte at a time on machines that need strict alignment. So "packed" mainly gives large and slow code. An example of this affecting core code is "struct ip" in netinet. This struct is naturally packed to give good alignment, so declaring it as "packed" seems to do nothing more than give large and slow code. I checked that it gives large and slow code on ia64's: "struct in_addr ip_src" at the end of struct ip is naturally aligned (the struct size is 20, and ip_src is in 4 bytes at the end, and everything has good alignment provided the beginning of the struct does). With the packed attribute, gcc has to access even this naturally aligned struct 1 byte at a type. I checked that it does this for loads. It uses 4 1-byte loads, and some ORs and presumably some shifts to reassemble the 4 bytes. Bruce
State Changed From-To: open->feedback This is believed to have been fixed by this commit: revision 1.21 date: 2005/05/27 12:25:42; author: rwatson; state: Exp; lines: +0 -3 Back out ipx.h:1.18, which introduced a Linux API compatibility field in the ipx_net data structure. Doing so introduced a stronger alignment requirement for the address structure, which in turn propagated into other dependent data structures, which turns out not to be suported by the available IPX source code. As a result, a number of user space applications, such as IPX routing components, failed to operate correctly. RELENG_5_3 candidate? PRs: 74059, 80266 Pointy hat to: bms Fix by: bde Tested by: Keith White <Keith dot White at site dot uottawa dot ca> MFC after: 1 week Suffering: great Could you confirm that (i.e., that this is corrected in 5.4 and later?). Thanks,
State Changed From-To: feedback->closed Transition to closed as this problem is believed fixed in more recent FreeBSD versions. If you are able to reproduce this problem still, please let me know and I can reopen the PR. Thanks for the report!