On Sat, Aug 1, 2020 at 8:46 AM Xie He <xie.he.0...@gmail.com> wrote: > > 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?
The kernel interface cannot be changed. If packet sockets used to pass the first byte up to userspace, they have to continue to do so. So I think you can limit the header_ops to only dev_hard_header. > 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. Fixes should be small and targeted. Any larger refactoring is best addressed in a separate net-next patch. > > 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!
