lkml.org 
[lkml]   [2020]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len
    On Sat, Aug 1, 2020 at 8:46 AM Xie He <xie.he.0141@gmail.com> wrote:
    >
    > On Fri, Jul 31, 2020 at 7:33 PM Willem de Bruijn
    > <willemdebruijn.kernel@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!

    \
     
     \ /
      Last update: 2020-08-01 15:31    [W:2.463 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site