Re: [PATCH net 2/2] net/af_packet: fix tx skb network header on SOCK_RAW sockets over VLAN device

[Willem de Bruijn]

On Tue, Jan 10, 2023 at 2:38 PM Hervé Boisse <admin@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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
>