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

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

[Willem de Bruijn]

On Mon, Apr 18, 2022 at 12:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Flavio reported that the kernel drops GSO VLAN tagged packet if it's
> created with socket(AF_PACKET, SOCK_RAW, 0) plus virtio_net_hdr.
>
> The reason is AF_PACKET doesn't adjust the skb network header if there is
> a VLAN tag. Then after virtio_net_hdr_set_proto() called, the skb->protocol
> will be set to ETH_P_IP/IPv6. And in later inet/ipv6_gso_segment() the skb
> is dropped as network header position is invalid.
>
> Fix it by adjusting network header position in packet_parse_headers()
> and move the function before calling virtio_net_hdr_* functions.
>
> I also moved skb->no_fcs setting upper to make all skb setting together
> and keep consistence of function packet_sendmsg_spkt().
>
> No need to update tpacket_snd() as it calls packet_parse_headers() in
> tpacket_fill_skb() before calling virtio_net_hdr_* functions.
>
> Fixes: 75c65772c3d1 ("net/packet: Ask driver for protocol if not provided by user")
> Reported-by: Flavio Leitner <fbl@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Strictly speaking VLAN tagged GSO packets have never been supported.
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.

> ---
>  net/packet/af_packet.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 002d2b9c69dd..cfdbda28ef82 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1924,12 +1924,20 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>
>  static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  {
> +       int depth;
> +
>         if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
>             sock->type == SOCK_RAW) {
>                 skb_reset_mac_header(skb);
>                 skb->protocol = dev_parse_header_protocol(skb);
>         }
>
> +       /* Move network header to the right position for VLAN tagged packets */
> +       if (likely(skb->dev->type == ARPHRD_ETHER) &&
> +           eth_type_vlan(skb->protocol) &&
> +           __vlan_get_protocol(skb, skb->protocol, &depth) != 0)
> +               skb_set_network_header(skb, depth);
> +
>         skb_probe_transport_header(skb);
>  }
>
> @@ -3047,6 +3055,11 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)

>         skb->mark = sockc.mark;
>         skb->tstamp = sockc.transmit_time;
>
> +       if (unlikely(extra_len == 4))
> +               skb->no_fcs = 1;
> +
> +       packet_parse_headers(skb, sock);
> +
>         if (has_vnet_hdr) {
>                 err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
>                 if (err)
> @@ -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.

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.

Because this patch touches many other packets besides the ones
intended, I am a bit concerned about unintended consequences. Perhaps
stretching the definition of the flags to include VLAN is acceptable
(unlike outright tunnels), but even then I would suggest for net-next.

>         err = po->xmit(skb);
>         if (unlikely(err != 0)) {
>                 if (err > 0)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Willem de Bruijn]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 19, 2022 at 09:56:02AM -0400, Willem de Bruijn wrote:
> > >
> > > 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.

Hmm I don't know offhand which duplication this refers to specifically
so maybe it's not worth addressing specifically but generally not
cleaning up code just because of backports seems wrong ...

> > > 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.

Yes, it's a UAPI extension, not to be done lightly. In this case IIUC
gso_type in the header is only u8 - 8 bits and 5 of these are already
used.  So I don't think the virtio TC will be all that happy to burn up
a bit unless a clear benefit can be demonstrated. 

I agree with the net-next proposal, I think it's more a feature than a
bugfix. In particular I think a Fixes tag can also be dropped in that
IIUC GSO for vlan packets didn't work even before that commit - right?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jason Wang]

On Wed, Apr 20, 2022 at 9:00 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Apr 19, 2022 at 10:26:09AM -0400, Michael S. Tsirkin wrote:
> > > > 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.
> >
> > Hmm I don't know offhand which duplication this refers to specifically
> > so maybe it's not worth addressing specifically but generally not
> > cleaning up code just because of backports seems wrong ...
>
> Yes, packet_snd() and tpacket_snd() share same addr/msg checking logic that
> I think we can clean up.
>
> > > > > 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.
> >
> > Yes, it's a UAPI extension, not to be done lightly. In this case IIUC
> > gso_type in the header is only u8 - 8 bits and 5 of these are already
> > used.  So I don't think the virtio TC will be all that happy to burn up
> > a bit unless a clear benefit can be demonstrated.
> >
> > I agree with the net-next proposal, I think it's more a feature than a
> > bugfix. In particular I think a Fixes tag can also be dropped in that
> > IIUC GSO for vlan packets didn't work even before that commit - right?

It should work, we initialize vlan_features since ("4fda830263c5
virtio-net: initialize vlan_features").

What we don't support is vlan offload.

>
> Right. virtio_net_hdr GSO with vlan doesn't work before.

It doesn't work since we don't support that feature.

> I will post this to net-next.

If you want to do that, you need a spec patch as well.

Thanks

>
> Thanks
> Hangbin
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/2] virtio_net: check L3 protocol for VLAN packets

[Willem de Bruijn]

On Mon, Apr 18, 2022 at 12:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> For gso packets, virtio_net_hdr_to_skb() will check the protocol via
> virtio_net_hdr_match_proto(). But a packet may come from a raw socket
> with a VLAN tag. Checking the VLAN protocol for virtio net_hdr makes no
> sense. Let's check the L3 protocol if it's a VLAN packet.
>
> Make the virtio_net_hdr_match_proto() checking for all skbs instead of
> only skb without protocol setting.
>
> Also update the data, protocol parameter for
> skb_flow_dissect_flow_keys_basic() as the skb->protocol may not IP or IPv6.
>
> Fixes: 7e5cced9ca84 ("net: accept UFOv6 packages in virtio_net_hdr_to_skb")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  include/linux/virtio_net.h | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index a960de68ac69..97b4f9680786 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -3,6 +3,7 @@
>  #define _LINUX_VIRTIO_NET_H
>
>  #include <linux/if_vlan.h>
> +#include <uapi/linux/if_arp.h>
>  #include <uapi/linux/tcp.h>
>  #include <uapi/linux/udp.h>
>  #include <uapi/linux/virtio_net.h>
> @@ -102,25 +103,36 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                  */
>                 if (gso_type && skb->network_header) {

This whole branch should not be taken by well formed packets. It is
inside the else clause of

       if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
          ..
       } else {

GSO packets should always request checksum offload. The fact that we
try to patch up some incomplete packets should not have to be expanded
if we expand support to include VLAN.

>                         struct flow_keys_basic keys;
> +                       __be16 protocol;
>
>                         if (!skb->protocol) {
> -                               __be16 protocol = dev_parse_header_protocol(skb);
> +                               protocol = dev_parse_header_protocol(skb);
>
>                                 if (!protocol)
>                                         virtio_net_hdr_set_proto(skb, hdr);
> -                               else if (!virtio_net_hdr_match_proto(protocol, hdr->gso_type))
> -                                       return -EINVAL;
>                                 else
>                                         skb->protocol = protocol;
> +                       } else {
> +                               protocol = skb->protocol;
>                         }
> +
> +                       /* Get L3 protocol if current protocol is VLAN */
> +                       if (likely(skb->dev->type == ARPHRD_ETHER) &&
> +                           eth_type_vlan(protocol))
> +                               protocol = vlan_get_protocol(skb);
> +
> +                       if (!virtio_net_hdr_match_proto(protocol, hdr->gso_type))
> +                               return -EINVAL;
> +
>  retry:
>                         if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> -                                                             NULL, 0, 0, 0,
> -                                                             0)) {
> +                                                             skb->data, protocol,
> +                                                             skb_network_offset(skb),
> +                                                             skb_headlen(skb), 0)) {
>                                 /* UFO does not specify ipv4 or 6: try both */
>                                 if (gso_type & SKB_GSO_UDP &&
> -                                   skb->protocol == htons(ETH_P_IP)) {
> -                                       skb->protocol = htons(ETH_P_IPV6);
> +                                   protocol == htons(ETH_P_IP)) {
> +                                       protocol = htons(ETH_P_IPV6);
>                                         goto retry;
>                                 }
>                                 return -EINVAL;
> --
> 2.35.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/2] virtio_net: check L3 protocol for VLAN packets

[Willem de Bruijn]

On Mon, Apr 18, 2022 at 11:14 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Mon, Apr 18, 2022 at 11:40:44AM -0400, Willem de Bruijn wrote:
> > On Mon, Apr 18, 2022 at 12:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > For gso packets, virtio_net_hdr_to_skb() will check the protocol via
> > > virtio_net_hdr_match_proto(). But a packet may come from a raw socket
> > > with a VLAN tag. Checking the VLAN protocol for virtio net_hdr makes no
> > > sense. Let's check the L3 protocol if it's a VLAN packet.
> > >
> > > Make the virtio_net_hdr_match_proto() checking for all skbs instead of
> > > only skb without protocol setting.
> > >
> > > Also update the data, protocol parameter for
> > > skb_flow_dissect_flow_keys_basic() as the skb->protocol may not IP or IPv6.
> > >
> > > Fixes: 7e5cced9ca84 ("net: accept UFOv6 packages in virtio_net_hdr_to_skb")
> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > ---
> > >  include/linux/virtio_net.h | 26 +++++++++++++++++++-------
> > >  1 file changed, 19 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index a960de68ac69..97b4f9680786 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -3,6 +3,7 @@
> > >  #define _LINUX_VIRTIO_NET_H
> > >
> > >  #include <linux/if_vlan.h>
> > > +#include <uapi/linux/if_arp.h>
> > >  #include <uapi/linux/tcp.h>
> > >  #include <uapi/linux/udp.h>
> > >  #include <uapi/linux/virtio_net.h>
> > > @@ -102,25 +103,36 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                  */
> > >                 if (gso_type && skb->network_header) {
> >
> > This whole branch should not be taken by well formed packets. It is
> > inside the else clause of
> >
> >        if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> >           ..
> >        } else {
> >
> > GSO packets should always request checksum offload. The fact that we
> > try to patch up some incomplete packets should not have to be expanded
> > if we expand support to include VLAN.
>
> Hi Willem,
>
> I'm not sure if I understand correctly. Do you mean we don't need to check
> L3 protocols for VLAN packet without NEEDS_CSUM flag? Which like
>
> if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>         ...
> } else if (!eth_type_vlan(skb->protocol)) {
>         ...
> }

Segmentation offload requires checksum offload. Packets that request
GSO but not NEEDS_CSUM are an aberration. We had to go out of our way
to handle them because the original implementation did not explicitly
flag and drop these. But we should not extend that to new types.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/2] virtio_net: check L3 protocol for VLAN packets

[Willem de Bruijn]

On Tue, Apr 19, 2022 at 9:16 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Hi Willem,
>
> On Tue, Apr 19, 2022 at 09:52:46AM -0400, Willem de Bruijn wrote:
> > Segmentation offload requires checksum offload. Packets that request
>
> OK, makes sense.
>
> > GSO but not NEEDS_CSUM are an aberration. We had to go out of our way
> > to handle them because the original implementation did not explicitly
> > flag and drop these. But we should not extend that to new types.
>
> So do you mean, the current gso types are enough, we should not extend to
> handle VLAN headers if no NEEDS_CSUM flag. This patch can be dropped, right?

That's right.

> Although I don't understand why we should not extend to support VLAN GSO.
> I'm OK if you think this patch should be dropped when I re-post patch 1/2 to
> net-next.
>
> Thanks
> Hangbin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization