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

[Hangbin Liu <liuhangbin@gmail.com>]

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)) {
	...
}

Thanks
Hangbin