On Fri, Jul 31, 2020 at 7:33 PM Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote: > > I quickly scanned the main x.25 datapath code. Specifically > x25_establish_link, x25_terminate_link and x25_send_frame. These all > write this 1 byte header. It appears to be an in-band communication > means between the network and data link layer, never actually ending > up on the wire?
Yes, this 1-byte header is just a "fake" header that is only for communication between the network layer and the link layer. It never ends up on wire. I think we can think of it as the Ethernet header for Wifi drivers. Although Wifi doesn't actually use the Ethernet header, Wifi drivers use a "fake" Ethernet header to communicate with code outside of the driver. From outside, it appears that Wifi drivers use the Ethernet header. > > The best solution might be to implement header_ops for X.25 drivers > > and let dev_hard_header create this 1-byte header, so that > > hard_header_len can equal to the header length created by > > dev_hard_header. This might be the best way to fit the logic of > > af_packet.c. But this requires changing the interface of X.25 drivers > > so it might be a big change. > > Agreed. Actually I tried this solution today. It was easier to implement than I originally thought. I implemented header_ops to make dev_hard_header generate the 1-byte header. And when receiving, (according to the requirement of af_packet.c) I pulled this 1-byte header before submitting the packet to upper layers. Everything worked fine, except one issue: When receiving, af_packet.c doesn't handle 0-sized packets well. It will drop them. This causes an AF_PACKET/DGRAM socket to receive no indication when it is connected or disconnected. Do you think this is a problem? Actually I'm also afraid that future changes in af_packet.c will make 0-sized packets not able to pass when sending as well. > Either lapbeth_xmit has to have a guard against 0 byte packets before > reading skb->data[0], or packet sockets should not be able to generate > those (is this actually possible today through PF_PACKET? not sure) > > If SOCK_DGRAM has to always select one of the three values (0x00: > data, 0x01: establish, 0x02: terminate) the first seems most sensible. > Though if there is no way to establish a connection with > PF_PACKET/SOCK_DGRAM, that whole interface may still be academic. > Maybe eventually either 0x00 or 0x01 could be selected based on > lapb->state.. That however is out of scope of this fix. Yes, I think the first solution may be better, because we need to have a way to drop 0-sized DGRAM packets (as long as we need to include the 1-byte header when sending DGRAM packets) and I'm not aware af_packet.c can do this. Yes, I think maybe the best way is to get rid of the 1-byte header completely and use other ways to ask the driver to connect or disconnect, or let it connect and disconnect automatically. > Normally a fix should aim to have a Fixes: tag, but all this code > precedes git history, so that is not feasible here. Thanks for pointing this out!
