Re: [PATCH v2] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len

[Willem de Bruijn]

On Sat, Aug 1, 2020 at 8:46 AM Xie He <xie.he.0141@xxxxxxxxx> wrote:
>
> On Fri, Jul 31, 2020 at 7:33 PM Willem de Bruijn
> <willemdebruijn.kernel@xxxxxxxxx> 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!