linux-kernel - Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200904151441.27c97d80@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 4 Sep 2020 15:14:41 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Xie He <xie.he.0141@...il.com>
Cc:     Krzysztof Halasa <khc@...waw.pl>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Martin Schiller <ms@....tdt.de>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for
 PVC devices

On Wed,  2 Sep 2020 17:06:58 -0700 Xie He wrote:
> PVC devices are virtual devices in this driver stacked on top of the
> actual HDLC device. They are the devices normal users would use.
> PVC devices have two types: normal PVC devices and Ethernet-emulating
> PVC devices.
> 
> When transmitting data with PVC devices, the ndo_start_xmit function
> will prepend a header of 4 or 10 bytes. Currently this driver requests
> this headroom to be reserved for normal PVC devices by setting their
> hard_header_len to 10. However, this does not work when these devices
> are used with AF_PACKET/RAW sockets. Also, this driver does not request
> this headroom for Ethernet-emulating PVC devices (but deals with this
> problem by reallocating the skb when needed, which is not optimal).
> 
> This patch replaces hard_header_len with needed_headroom, and set
> needed_headroom for Ethernet-emulating PVC devices, too. This makes
> the driver to request headroom for all PVC devices in all cases.

Since this is a tunnel protocol on top of HDLC interfaces, and
hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually 
set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if 
hdlc devices actually prepend 16 bytes of header, though.

CC: Willem as he was reviewing your similar patch recently.

> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index 9acad651ea1f..12b35404cd8e 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev)
>  {
>  	dev->type = ARPHRD_DLCI;
>  	dev->flags = IFF_POINTOPOINT;
> -	dev->hard_header_len = 10;
> +	dev->hard_header_len = 0;

Is there a need to set this to 0? Will it not be zero after allocation?

>  	dev->addr_len = 2;
>  	netif_keep_dst(dev);
>  }
> @@ -1093,6 +1093,7 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type)
>  	dev->mtu = HDLC_MAX_MTU;
>  	dev->min_mtu = 68;
>  	dev->max_mtu = HDLC_MAX_MTU;
> +	dev->needed_headroom = 10;
>  	dev->priv_flags |= IFF_NO_QUEUE;
>  	dev->ml_priv = pvc;
>