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

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

[Xie He]

In net/packet/af_packet.c, the function packet_snd first reserves a
headroom of length (dev->hard_header_len + dev->needed_headroom).
Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header,
which calls dev->header_ops->create, to create the link layer header.
If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of
length (dev->hard_header_len), and assumes the user to provide the
appropriate link layer header.

So according to the logic of af_packet.c, dev->hard_header_len should
be the length of the header that would be created by
dev->header_ops->create.

However, this driver doesn't provide dev->header_ops, so logically
dev->hard_header_len should be 0.

So we should use dev->needed_headroom instead of dev->hard_header_len
to request necessary headroom to be allocated.

Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/lapbether.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index b2868433718f..34cf6db89912 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -305,6 +305,7 @@ static void lapbeth_setup(struct net_device *dev)
 	dev->netdev_ops	     = &lapbeth_netdev_ops;
 	dev->needs_free_netdev = true;
 	dev->type            = ARPHRD_X25;
+	dev->hard_header_len = 0;
 	dev->mtu             = 1000;
 	dev->addr_len        = 0;
 }
@@ -331,7 +332,8 @@ static int lapbeth_new_device(struct net_device *dev)
 	 * then this driver prepends a length field of 2 bytes,
 	 * then the underlying Ethernet device prepends its own header.
 	 */
-	ndev->hard_header_len = -1 + 3 + 2 + dev->hard_header_len;
+	ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len
+					   + dev->needed_headroom;
 
 	lapbeth = netdev_priv(ndev);
 	lapbeth->axdev = ndev;
-- 
2.25.1

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

[Xie He]

Hi Cong Wang,

I'm wishing to change a driver from using "hard_header_len" to using
"needed_headroom" to declare its needed headroom. I submitted a patch
and it is decided it needs to be reviewed. I see you participated in
"hard_header_len vs needed_headroom" discussions in the past. Can you
help me review this patch? Thanks!

The patch is at:
http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@gmail.com/

In my understanding, hard_header_len should be the length of the header
created by dev_hard_header. Any additional headroom needed should be
declared in needed_headroom instead of hard_header_len. I came to this
conclusion by examining the logic of net/packet/af_packet.c:packet_snd.

What do you think?

Thanks!

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

[Cong Wang]

Hello,

On Mon, Jul 27, 2020 at 12:41 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Hi Cong Wang,
>
> I'm wishing to change a driver from using "hard_header_len" to using
> "needed_headroom" to declare its needed headroom. I submitted a patch
> and it is decided it needs to be reviewed. I see you participated in
> "hard_header_len vs needed_headroom" discussions in the past. Can you
> help me review this patch? Thanks!
>
> The patch is at:
> http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@gmail.com/
>
> In my understanding, hard_header_len should be the length of the header
> created by dev_hard_header. Any additional headroom needed should be
> declared in needed_headroom instead of hard_header_len. I came to this
> conclusion by examining the logic of net/packet/af_packet.c:packet_snd.

I am not familiar with this WAN driver, but I suggest you to look at
the following commit, which provides a lot of useful information:

commit 9454f7a895b822dd8fb4588fc55fda7c96728869
Author: Brian Norris <briannorris@chromium.org>
Date:   Wed Feb 26 16:05:11 2020 -0800

    mwifiex: set needed_headroom, not hard_header_len

    hard_header_len provides limitations for things like AF_PACKET, such
    that we don't allow transmitting packets smaller than this.

    needed_headroom provides a suggested minimum headroom for SKBs, so that
    we can trivally add our headers to the front.

    The latter is the correct field to use in this case, while the former
    mostly just prevents sending small AF_PACKET frames.

    In any case, mwifiex already does its own bounce buffering [1] if we
    don't have enough headroom, so hints (not hard limits) are all that are
    needed.

    This is the essentially the same bug (and fix) that brcmfmac had, fixed
    in commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
    additional header space").

    [1] mwifiex_hard_start_xmit():
            if (skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN) {
            [...]
                    /* Insufficient skb headroom - allocate a new skb */

Hope this helps.

Thanks.

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

[Xie He]

Hi Brian Norris,

I'm wishing to change a driver from using "hard_header_len" to using
"needed_headroom" to declare its needed headroom. I submitted a patch
and it is decided it needs to be reviewed. I see you submitted a similar
patch in the past. So I think you might be able to help review my patch.
Can you help review my patch? Thanks!

The patch is at:
http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@gmail.com/

As you have noted in the commit message of your patch, hard_header_len
has special meaning in AF_PACKET so we need to use needed_headroom
instead when we just need to reserve some header space. I believe the
value of hard_header_len should be the same as the length of the header
created by dev_hard_header.

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

[Brian Norris]

(Reviewing as requested; I'm not familiar with this driver either, or
really any WAN driver. It also seems that hard_header_len vs.
needed_headroom aren't very well documented, and even I can't guarantee
I understand them completely. So take my thoughts with a grain of salt.)

Hi,

On Sun, Jul 26, 2020 at 04:05:24AM -0700, Xie He wrote:
> In net/packet/af_packet.c, the function packet_snd first reserves a
> headroom of length (dev->hard_header_len + dev->needed_headroom).
> Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header,
> which calls dev->header_ops->create, to create the link layer header.
> If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of
> length (dev->hard_header_len), and assumes the user to provide the
> appropriate link layer header.
> 
> So according to the logic of af_packet.c, dev->hard_header_len should
> be the length of the header that would be created by
> dev->header_ops->create.

I believe I'm with you up to here, but:

> However, this driver doesn't provide dev->header_ops, so logically
> dev->hard_header_len should be 0.

I'm not clear on this part.

What's to say you shouldn't be implementing header_ops instead? Note
that with WiFi drivers, they're exposing themselves as ARPHRD_ETHER, and
only the Ethernet headers are part of the upper "protocol" headers. So
my patch deferred to the eth headers.

What is the intention with this X25 protocol? I guess the headers added
in lapbeth_data_transmit() are supposed to be "invisible", as with this
note in af_packet.c?

   - if device has no dev->hard_header routine, it adds and removes ll header
     inside itself. In this case ll header is invisible outside of device,
     but higher levels still should reserve dev->hard_header_len.

If that's the case, then yes, I believe this patch should be correct.

Brian

> So we should use dev->needed_headroom instead of dev->hard_header_len
> to request necessary headroom to be allocated.
> 
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

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

[Xie He]

Thank you for your detailed review, Brian!

I guess we have the same understanding on the "hard_header_len vs
needed_headroom" part. I agree it is not well documented and is also
confusing to driver developers. I didn't understand it either until I
looked at af_packet.c.

On Tue, Jul 28, 2020 at 12:52 PM -0700
Brian Norris <briannorris@chromium.org> wrote:
>
> What's to say you shouldn't be implementing header_ops instead? Note
> that with WiFi drivers, they're exposing themselves as ARPHRD_ETHER, and
> only the Ethernet headers are part of the upper "protocol" headers. So
> my patch deferred to the eth headers.
>
> What is the intention with this X25 protocol? I guess the headers added
> in lapbeth_data_transmit() are supposed to be "invisible", as with this
> note in af_packet.c?
>
>    - if device has no dev->hard_header routine, it adds and removes ll header
>      inside itself. In this case ll header is invisible outside of device,
>      but higher levels still should reserve dev->hard_header_len.
>
> If that's the case, then yes, I believe this patch should be correct.

This driver is not intended to be used with IPv4 or IPv6 protocols,
but is intended to be used with a special "X.25" protocol. That's the
reason the device type is ARPHRD_X25. I used "grep" in the X.25
network layer code (net/x25) and I found there's nowhere
"dev_hard_header" is called. I also used "grep" in all the X.25
drivers in the kernel (lapbether.c, x25_asy.c, hdlc_x25.c under
drivers/net/wan) and I found no driver implemented "header_ops". So I
think the X.25 networking code doesn't expect any header visible
outside of the device driver, and X.25 drivers should make their
headers invisible outside of them.

So I think hard_header_len should be 0 for all X.25 drivers, so that
they can be used correctly with af_packet.c.

I don't know if this sounds plausible to you. If it does, could you
please let me have your name in a "Reviewed_by" tag. It would be of
great help to have your support. Thanks!

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

[Brian Norris]

On Tue, Jul 28, 2020 at 6:42 PM Xie He <xie.he.0141@gmail.com> wrote:
> On Tue, Jul 28, 2020 at 12:52 PM -0700
> Brian Norris <briannorris@chromium.org> wrote:
> > What is the intention with this X25 protocol? I guess the headers added
> > in lapbeth_data_transmit() are supposed to be "invisible", as with this
> > note in af_packet.c?
...
> This driver is not intended to be used with IPv4 or IPv6 protocols,
> but is intended to be used with a special "X.25" protocol. That's the
> reason the device type is ARPHRD_X25. I used "grep" in the X.25
> network layer code (net/x25) and I found there's nowhere
> "dev_hard_header" is called. I also used "grep" in all the X.25

That well could just be a bug in net/x25/. But from context, it does
appear that X.25 does not intend to expose its headers to higher
layers.

> drivers in the kernel (lapbether.c, x25_asy.c, hdlc_x25.c under
> drivers/net/wan) and I found no driver implemented "header_ops". So I
> think the X.25 networking code doesn't expect any header visible
> outside of the device driver, and X.25 drivers should make their
> headers invisible outside of them.
>
> So I think hard_header_len should be 0 for all X.25 drivers, so that
> they can be used correctly with af_packet.c.
>
> I don't know if this sounds plausible to you. If it does, could you
> please let me have your name in a "Reviewed_by" tag. It would be of
> great help to have your support. Thanks!

Sure, I can do that:

Reviewed-by: Brian Norris <briannorris@chromium.org>

I guess x25 is basically an abandoned project, if you're coming to me for this?

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

[Xie He]

On Thu, Jul 30, 2020 at 5:24 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Sure, I can do that:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

Thank you so much for your review, Brian!

> I guess x25 is basically an abandoned project, if you're coming to me for this?

Yes, it does seem to me that X.25 is unmaintained. I'm submitting
patches for it because I'm personally interested in X.25 and I want to
fix things that I find to be having issues. But it's very hard for me
to do so because it's hard to find reviewers for X.25 code. So I
really appreciate that you review this patch. Thanks!

I don't know if it is the right thing to continue submitting patches
for X.25, or we should just keep the code as is. Maybe the kernel
community can have a discussion some time to decide what to do with
it.