Re: [PATCH] net/packet: fix network header offset-VLAN raw packets on VLAN subinterfaces

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

Junnan Zhang wrote:
> Hi Willem,
> 
> Thank you for the review.
> 
> > > AF_PACKET SOCK_RAW reserves dev->hard_header_len bytes of headroom. For
> > > VLAN subinterfaces, hard_header_len VLAN tag space (18 bytes)
> >
> > Does it?
> >
> > vlan_dev_init:
> >
> >         dev->hard_header_len = real_dev->hard_header_len;
> 
> You are right that this is only true when VLAN hardware offloading is
> available, i.e. vlan_hw_offload_capable() returns true and vlan_dev_init()
> takes the first branch. The bug I am fixing only happens in the else
> branch:
> 
>         dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;
> 
> I observed it on a virtio_net device, which advertises
> NETIF_F_HW_VLAN_CTAG_FILTER but not IF_F_HW_VLAN_CTAG_TX. So any VLAN
> subinterface created on top of it uses software VLAN tag insertion and
> has hard_header_len = 18 while min_header_len stays at 14.
> 
> > > while min_header_len is the real Ethernet header length (14 bytes). When
> >
> > Which device did you observe this with?
> 
> virtio_net (in a KVM/QEMU guest).
> 
> > > userspace sends a standard untagged Ethernet frame through a VLAN
> > > subinterface, packet_parse_headers() only correct_header for
> > > VLAN-tagged frames. For non-VLAN frames it leaves network_header at
> > > hard_header_len, so the IP header is found 4 bytes too late and
> > > inet_gso_segment() fails with -EINVAL.
> >
> > Which path did you observe generating these untagged packets through
> > a VLAN interface?
> 
> The reproducer is an AF_PACKET SOCK_RAW socket bound to the VLAN
> subinterface, with PACKET_VNET_HDR enabled. Userspace sends a large
> IPv4/TCP frame that exceeds the path MTU; the virtio-net header in the
> packet sets gso_type, so the skb goes through GSO. The userspace frame
> contains a plain Ethernet + IP + TCP layout, without a VLAN tag. The VLAN
> sub inserts the 802.1Q tag in vlan_dev_hard_start_xmit().
> 
> Before the fix, packet_snd() leaves network_header at base +
> hard_header_len (18), while the real IP header starts at base + 14 + 14 =
> base + 28. network_header points 4 bytes past the IP header, so
> inet_gso_segment() gets a misaligned ip_hdr(skb) and returns -EINVAL.

Why is the real length 14 + 14 == 28?
Where does the second 14 come from?

> > > +	bool has_vlan;
> >
> > nit: confusing variable, combining test on packet and device.
> 
> Ag. In v2 I will restructure the function to test dev->type once and
> use a clearly packet-only variable. For example:
> 
>         if (likely(skb->dev->type == ARPHRD_ETHER)) {
>                 bool is_vlan = eth_type_vlan(skb->protocol);
> 
>                 if (!is_vlan && sock->type == SOCK_RAW &&
>                     skb->dev->min_header_len < skb->dev->hard_header_len)

This is the hint that this is a vlan device with software VLAN tag
insertion? Technically, it might apply to other variable length
header devices too.
>                         skb_set_network_header(skb, skb->dev->min_header_len);
> 
>                 skb_probe_transport_header(skb);
> 
>                 if (is_vlan &&
>                     vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
>                         skb_set_network_header(skb, depth);
>         } else {
>                 skb_probe_transport_header(skb);
>         }
> 
> > > +	    likely(skb->dev->type == ARPHRD_ETHER) &&
> >