[PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

The underlying Ethernet device may request necessary tailroom to be
allocated by setting needed_tailroom. This driver should also set
needed_tailroom to request the tailroom needed by the underlying
Ethernet device to be allocated.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Martin Schiller <ms@dev.tdt.de>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/lapbether.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index 1ea15f2123ed..cc297ea9c6ec 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -340,6 +340,7 @@ static int lapbeth_new_device(struct net_device *dev)
 	 */
 	ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len
 					   + dev->needed_headroom;
+	ndev->needed_tailroom = dev->needed_tailroom;
 
 	lapbeth = netdev_priv(ndev);
 	lapbeth->axdev = ndev;
-- 
2.25.1

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Willem de Bruijn]

On Sat, Aug 8, 2020 at 7:53 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> The underlying Ethernet device may request necessary tailroom to be
> allocated by setting needed_tailroom. This driver should also set
> needed_tailroom to request the tailroom needed by the underlying
> Ethernet device to be allocated.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  drivers/net/wan/lapbether.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
> index 1ea15f2123ed..cc297ea9c6ec 100644
> --- a/drivers/net/wan/lapbether.c
> +++ b/drivers/net/wan/lapbether.c
> @@ -340,6 +340,7 @@ static int lapbeth_new_device(struct net_device *dev)
>          */
>         ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len
>                                            + dev->needed_headroom;
> +       ndev->needed_tailroom = dev->needed_tailroom;

Does this solve an actual observed bug?

In many ways lapbeth is similar to tunnel devices. This is not common.

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

On Sun, Aug 9, 2020 at 1:48 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Does this solve an actual observed bug?
>
> In many ways lapbeth is similar to tunnel devices. This is not common.

Thank you for your comment!

This doesn't solve a bug observed by me. But I think this should be
necessary considering the logic of the code.

Using "grep", I found that there were indeed Ethernet drivers that set
needed_tailroom. I found it was set in these files:
    drivers/net/ethernet/sun/sunvnet.c
    drivers/net/ethernet/sun/ldmvsw.c
Setting needed_tailroom may be necessary for this driver to run those
Ethernet devices.

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Willem de Bruijn]

On Sun, Aug 9, 2020 at 7:12 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sun, Aug 9, 2020 at 1:48 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Does this solve an actual observed bug?
> >
> > In many ways lapbeth is similar to tunnel devices. This is not common.
>
> Thank you for your comment!
>
> This doesn't solve a bug observed by me. But I think this should be
> necessary considering the logic of the code.
>
> Using "grep", I found that there were indeed Ethernet drivers that set
> needed_tailroom. I found it was set in these files:
>     drivers/net/ethernet/sun/sunvnet.c
>     drivers/net/ethernet/sun/ldmvsw.c
> Setting needed_tailroom may be necessary for this driver to run those
> Ethernet devices.

What happens when a tunnel device passes a packet to these devices?
That will also not have allocated the extra tailroom. Does that cause
a bug?

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

On Mon, Aug 10, 2020 at 12:32 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> What happens when a tunnel device passes a packet to these devices?
> That will also not have allocated the extra tailroom. Does that cause
> a bug?

I looked at the code in net/ipv4/ip_tunnel.c. It indeed appeared to me
that it didn't take needed_tailroom into consideration. However it
does take needed_headroom into consideration through the macro
LL_RESERVED_SPACE. I think it would be better for it to take
needed_tailroom into consideration, too.

However, looking at the comment of needed_tailroom in
include/linux/netdevice.h, it says "Extra tailroom the hardware may
need, but not in all cases can this be guaranteed". So if we take this
comment as the spec, we can consider this to be not a bug. The reason
the author of this comment said so, might be that he wanted to add
needed_tailroom to solve some problems, but he was not able to change
all code to take needed_tailroom into consideration, so he wrote in
the comment saying that it was not necessary to always guarantee
needed_tailroom.

If we take this comment as the spec, to prevent bugs, any driver that
sets needed_tailroom must always check (and re-allocate if necessary)
before using the tailroom.

However, I still think it would be better to always take into
consideration needed_tailroom (and needed_headroom, too), so that
eventually we can remove the words of "but not in all cases can this
be guaranteed" from the comment. That would make the code more logical
and consistent.

Thank you for raising this important question about needed_tailroom!

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

I took some time to look at the history of needed_tailroom. I found it
was added in this commit:
f5184d267c1a (net: Allow netdevices to specify needed head/tailroom)

The author tried to make use of needed_tailroom at various places in
the kernel by replacing the macro LL_RESERVED_SPACE with his new macro
LL_ALLOCATED_SPACE.

However, the macro LL_ALLOCATED_SPACE was later found to have
problems. So it was removed 3 years later and was replaced by explicit
handling of needed_tailroom. See:
https://lkml.org/lkml/2011/11/18/198

So maybe only those places considered by these two authors have taken
needed_tailroom into account.

Other places might not have taken needed_tailroom into account because
of the rarity of the usage of needed_tailroom.

The second author also said in the commit message of his Patch 5/6
(which changes af_packet.c), that:
    While auditing LL_ALLOCATED_SPACE I noticed that packet_sendmsg_spkt
    did not include needed_tailroom when allocating an skb.  This isn't
    a fatal error as we should always tolerate inadequate tail room but
    it isn't optimal.

This shows not taking needed_tailroom into account is not a bug but
it'd be better to take it into account.

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

Hi Guillaume Nault,

I'm currently trying to fix a driver's "needed_tailroom" setting. This
driver is a virtual driver stacked on top of Ethernet devices (similar
to pppoe). I believe its needed_tailroom setting should take into
account the underlying Ethernet device's needed_tailroom. So I
submitted this patch. I see you previously also did a change related
to needed_tailroom to pppoe. Can you help review this patch? Thank you
so much!

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

Thanks!
Xie He

Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom

[Xie He]

Hi Martin Schiller,

Can you help review this patch? Thanks! It is a very simple patch that
adds the "needed_tailroom" setting for this driver.

Thank you!
Xie He