[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+FuTSdq5E7GMUghfoXMrs5_muv6VbhFym8DEhw=rAgStuvkQg@mail.gmail.com>
Date: Thu, 12 Jan 2023 11:04:58 -0500
From: Willem de Bruijn <willemb@...gle.com>
To: Hervé Boisse <admin@...geek.ovh>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/2] net/af_packet: fix tx skb network header on
SOCK_RAW sockets over VLAN device
On Tue, Jan 10, 2023 at 2:38 PM Hervé Boisse <admin@...geek.ovh> wrote:
>
> When an application sends a packet on a SOCK_RAW socket over a VLAN device,
> there is a possibility that the skb network header is incorrectly set.
>
> The issue happens when the device used to send the packet is a VLAN device
> whose underlying device has no VLAN tx hardware offloading support.
> In that case, the VLAN driver reports a LL header size increased by 4 bytes
> to take into account the tag that will be added in software.
>
> However, the socket user has no clue about that and still provides a normal
> LL header without tag.
This is arguably a mistake.
A process using PF_PACKET to write to a device must know the link layer type.
SOCK_RAW should prepare a header equivalent to dev_hard_header (which
prepares it for SOCK_DGRAM). vlan_dev_hard_header clearly adds both the
Ethernet header and VLAN tag.
If applications assume virtual VLAN device exposes an Ethernet link layer,
then net/8021q/vlan_dev.c needs to expose that, including that hard_header_len.
> This results in the network header of the skb being shifted 4 bytes too far
> in the packet. This shift makes tc classifiers fail as they point to
> incorrect data.
>
> Move network header right after underlying VLAN device LL header size
> without tag, regardless of hardware offloading support. That is, the
> expected LL header size from socket user.
>
> Signed-off-by: Hervé Boisse <admin@...geek.ovh>
> ---
> net/packet/af_packet.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c18274f65b17..be892fd498a6 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1933,6 +1933,18 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
> skb->protocol = dev_parse_header_protocol(skb);
> }
>
> + /* VLAN device may report bigger LL header size due to reserved room for
> + * tag on devices without hardware offloading support
> + */
> + if (is_vlan_dev(skb->dev) &&
> + (sock->type == SOCK_RAW || sock->type == SOCK_PACKET)) {
Let's also try very hard to avoid adding branches in the hot path for
cases this rare.
> + struct net_device *real_dev = vlan_dev_real_dev(skb->dev);
> +
> + depth = real_dev->hard_header_len;
> + if (pskb_may_pull(skb, depth))
> + skb_set_network_header(skb, depth);
> + }
> +
> /* Move network header to the right position for VLAN tagged packets */
> if (likely(skb->dev->type == ARPHRD_ETHER) &&
> eth_type_vlan(skb->protocol) &&
> --
> 2.38.2
>
Powered by blists - more mailing lists