[linux-tr] Token Ring Bug (This is good :-)).

["Mike Phillips" <phillim@amtrak.com>]

All,

I recently received an e-mail from Anonymous Coward (in the grand /. tradition), critiquing the token ring code.  (They are not quite as brave / stupid as I am and do not want to start a flame war )  I think they read the mailing list, so I would suggest we discuss these items on the list. Lines with -- xxxxx MLP are my comments on the mail.

My main suggestion to the mailer is to put your money where your mouth is and start posting patches ;-)

------ Original Mail starts here - MLP
I've been looking quite closely at the main token ring code 
( if_tr.h & /802/tr.c ), I'm running linux-2.2.3 & in my opinion
the code is really really bad, the drivers seem fine.
-- Wow - I though for a moment my driver was a problem - MLP

I'm currently trying to get the token ring code going on a large
network at work & to be quite honest I don't even understand how the code could
possibly work in the first place. 
-- I don't even understand how it works, period  - MLP

I've made some attempts to kludge/fix it up in the parts that I know
are broken & I'll post you back the fixes when I'm content that the stuff 
works better than the stuff already there.

Here are a list of known problems & things which I might have managed
to fix ( low hanging fruit ).

More than likely fixed:
1) The code was manipulating the source routing linked lists at
interrupt time & bottom half time & viewing it from the proc fs
at user time without acquiring an atomic lock of any kind & falling
though if it failed to get one, just disabling interrupts ( not smp safe ) 
in the rif_check_expire leading to possible crashes.

2) Somebody incorrectly quoting rfc 1042 limited all broadcasts 
( including arp ) to a source routed limited broadcast of zero hops,
( just getting the packet onto the local ring ).
How is this supposed to work on a large ring the guy obviously didn't 
know the difference between an ip broadcast & a hardware broadcast.
To do an IP broadcast ( or similar is in my opinion the most
complicated peice of code which needs to be implemented. I believe it should be
done in the following manner even though it is very cpu intensive & complex.

a) A complete token ring network topology map ( rings/bridges ) is
built up approximately once when a broadcast is required but at most once every 
10 minutes ( possibly tunable ).
This would require one the sniff a portion of all incoming token ring
packets remove the rif information & add this to a static table like the
following
typedef struct
{
  
  unsigned long __last_broadcasted_to; // jiffies
  unsigned ring:4;
  unsigned unused:3;
  unsigned valid:1;
} tr_broadcast_table __attribute__(packed);

tr_broadcast_table rif_br[4096]; 
4096 being the maximum number of rings supported by rfc1042 & 1749.
b) When a ip (or other protocol except arp ) broadcast is received 
a route containing at as many as possible of the least recently 
broadcasted to rings ( 8 or less limited by  rfc1042 ) is broadcast
 using a single route broadcast. This has the limitation that we can 
only hit 8 rings in a single shot but upper layer protocols will retry 
& eventually hit every known rings.

3) There was no maximum limit put into the size the source routing
information can grow to except by kmalloc :-). 

4) Cleaned up some general sloppyness with types
e.g. __u16 & unsigned short shouldn't be used interchangibly 
( a common problem in linux code )  
all the standards gaurantee sizewise is
char <= short <= int <= long <= long long
to strike the point home
a long is 8 bytes on an alpha, 
an int is 2 or 4 bytes on a 68k depending on who your talking to
& there are some antiques around where a char < 8 bits.

5) The rif_get_info was disabling interrupts for potentially several
miliseconds potentially leading to crashes or at very least have
several people wondering why their serial devices & other non dma devices going
bezerk once every 10 minutes.

Possible Fixes:
1) tr_header appears to return a header size different to the one sent
out on the network  ( I don't think this is correct ) tr_source_route generates a variable sized header.

Suspicions:
1) Does this stuff work with the XID & TEST commands coded in the 
/802 directory ?
2) Are the ac & fc fields set correctly for all protocols,
I know token uses them to encode priority information as well as
error control on the network ?
3) How could the tr_header code possibly work for IPX when it dosen't 
add IPX/token ring specific information into the header, except to a
linux box running an equally broken IPX protocol stack.
Quoting another source:
"
The main feature of this framing type is the SAP (Service
Advertising Protocol) fields, which
indicate the protocol type.  For Novell networks, these fields
are set to 0xe0, which indicates that the upper layer protocol
is IPX.  Ethernet 802.3 with 802.2 header frames have this
format:

 ---------------------------------------
 Header   Field                 Size
 ---------------------------------------
 802.3    destination address   6 octets
 802.3    source address        6 octets
 802.3    length                2 octets
 802.2    destination SAP       1 octet
 802.2    source SAP            1 octet
 802.2    control               1 octet
          data                  ...
 ---------------------------------------
"
( it looks very flaky for even the more common protocols ) ?
4) AIX 4.0 on some machines appears to have a canary at the pseudo
random packets the token ring stack is emitting ( when you ping from aix in
our lab it's socket buffer eventually fills up ).
5) The MTU is hardwired to 2000 how in the hell does this even half
work with ethernet bridging ( MTU 1516 or less ) ( read rfc1042 ).
-- That's the router / bridge's problem - MLP

Suggestions:
1) Add hooks/ function pointers to arp ( which can also be used by
other network hardware possibly atm ) which can add token ring routing 
info / delete it &print /proc information for the route. 
This I think would be a good idea because it is quite possible for tr_header & rebuild header to be called & we have no routing information for it & all we can do is transmit the packet on the local ring we also cannot be called unless arp or some other neighbour lookup mechanism has been called which means we are maintaining tons of redundant information ( 100k + on large networks ) in the token ring routing table. The only downside of this is if other neighbour lookup
mechanisms are in linux which use token ring ( of which I'm aware of none ) 
the hooks also need to be added to this code.
2 ) Make the network stacks support mtu's for different sockets
this way we can use the mtu's in a fashion recommended by ref 1042.

I'll send you my kludge/fixes once I'm content to let them out the
door. I'd rewrite the thing from scratch except I've other commitments & I
still possibly wouldn't do a good job & other people would curse & fire
insults at it :-).
-- Nah, post what you, when you have it, the linuxtr listers are big ang ugly enough to kernel panic their machines - MLP

--- End of original mail - MLP

Well, what do we think, if the poster is right then lets clean up the tr code, if they are wrong lets tell them why.

Mike
LTRP www.linuxtr.net