Message ID | 20200808175251.582781-1-xie.he.0141@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] drivers/net/wan/lapbether: Added needed_tailroom | expand |
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.
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.
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?
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!
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.
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
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
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;
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(+)