Re: [PATCH net 1/2] net/af_packet: adjust network header position for VLAN tagged packets

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Mon, Apr 18, 2022 at 11:02 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Mon, Apr 18, 2022 at 11:38:14AM -0400, Willem de Bruijn wrote:
> > Strictly speaking VLAN tagged GSO packets have never been supported.
>
> OK, I thought we just forgot to handle the VLAN header for RAW af socket.
> As in the later path skb_mac_gso_segment() deal with VLAN correctly.
>
> If you think this should be a new feature instead of fixes. I can remove the
> fixes tag and re-post it to net-next, as you said.
>
> > The only defined types are TCP and UDP over IPv4 and IPv6:
> >
> >   define VIRTIO_NET_HDR_GSO_TCPV4        1       /* GSO frame, IPv4 TCP (TSO) */
> >   define VIRTIO_NET_HDR_GSO_UDP          3       /* GSO frame, IPv4 UDP (UFO) */
> >   define VIRTIO_NET_HDR_GSO_TCPV6        4       /* GSO frame, IPv6 TCP */
> >
> > I don't think this is a bug, more a stretching of the definition of those flags.
>
> I think VLAN is a L2 header, so I just reset the network header position.
>
> I'm not familiar with virtio coded. Do you mean to add a new flag like VIRTIO_NET_HDR_GSO_VLAN?
> > > @@ -3055,11 +3068,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >                 virtio_net_hdr_set_proto(skb, &vnet_hdr);
> > >         }
> > >
> > > -       packet_parse_headers(skb, sock);
> > > -
> > > -       if (unlikely(extra_len == 4))
> > > -               skb->no_fcs = 1;
> > > -
> >
> > Moving packet_parse_headers before or after virtio_net_hdr_to_skb may
> > have additional subtle effects on protocol detection.
> >
> > I think it's probably okay, as tpacket_snd also calls in the inverse
> > order. But there have been many issues in this codepath.
>
> Yes
>
> >
> > We should also maintain feature consistency between packet_snd,
> > tpacket_snd and to the limitations of its feature set to
> > packet_sendmsg_spkt. The no_fcs is already lacking in tpacket_snd as
> > far as I can tell. But packet_sendmsg_spkt also sets it and calls
> > packet_parse_headers.
>
> Yes, I think we could fix the tpacket_snd() in another patch.
>
> There are also some duplicated codes in these *_snd functions.
> I think we can move them out to one single function.

Please don't refactor this code. It will complicate future backports
of stable fixes.

> > Because this patch touches many other packets besides the ones
> > intended, I am a bit concerned about unintended consequences. Perhaps
>
> Yes, makes sense.
>
> > stretching the definition of the flags to include VLAN is acceptable
> > (unlike outright tunnels), but even then I would suggest for net-next.
>
> As I asked, I'm not familiar with virtio code. Do you think if I should
> add a new VIRTIO_NET_HDR_GSO_VLAN flag? It's only a L2 flag without any L3
> info. If I add something like VIRTIO_NET_HDR_GSO_VLAN_TCPV4/TCPV6/UDP. That
> would add more combinations. Which doesn't like a good idea.

I would prefer a new flag to denote this type, so that we can be
strict and only change the datapath for packets that have this flag
set (and thus express the intent).

But the VIRTIO_NET_HDR types are defined in the virtio spec. The
maintainers should probably chime in.