Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230110191725.22675-1-admin@netgeek.ovh> <20230110191725.22675-2-admin@netgeek.ovh>
In-Reply-To: <20230110191725.22675-2-admin@netgeek.ovh>
From:   Willem de Bruijn <willemb@google.com>
Date:   Thu, 12 Jan 2023 11:04:58 -0500
Message-ID: <CA+FuTSdq5E7GMUghfoXMrs5_muv6VbhFym8DEhw=rAgStuvkQg@mail.gmail.com>
Subject: Re: [PATCH net 2/2] net/af_packet: fix tx skb network header on
 SOCK_RAW sockets over VLAN device
To:     =?UTF-8?Q?Herv=C3=A9_Boisse?= <admin@netgeek.ovh>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jan 10, 2023 at 2:38 PM Herv=C3=A9 Boisse <admin@netgeek.ovh> wrote=
:
>
> When an application sends a packet on a SOCK_RAW socket over a VLAN devic=
e,
> 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 devic=
e
> whose underlying device has no VLAN tx hardware offloading support.
> In that case, the VLAN driver reports a LL header size increased by 4 byt=
es
> 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 norm=
al
> LL header without tag.

This is arguably a mistake.

A process using PF_PACKET to write to a device must know the link layer typ=
e.
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 f=
ar
> 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=C3=A9 Boisse <admin@netgeek.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 *s=
kb, struct socket *sock)
>                 skb->protocol =3D dev_parse_header_protocol(skb);
>         }
>
> +       /* VLAN device may report bigger LL header size due to reserved r=
oom for
> +        * tag on devices without hardware offloading support
> +        */
> +       if (is_vlan_dev(skb->dev) &&
> +           (sock->type =3D=3D SOCK_RAW || sock->type =3D=3D 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 =3D vlan_dev_real_dev(skb->de=
v);
> +
> +               depth =3D 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 pack=
ets */
>         if (likely(skb->dev->type =3D=3D ARPHRD_ETHER) &&
>             eth_type_vlan(skb->protocol) &&
> --
> 2.38.2
>