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

[Junnan Zhang <zhangjn_dev@163.com>]

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.

> > +	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)
                        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) &&
>
> nit: repeat test, also included in that has_vlan

Yes, this is fixed by the above restructuring. ARPHRD_ETHER is tested 
only once.

I will update the commit message to make the "non-offload VLAN
subinterface" scope explicit, add the virtio_net observation and the
reproducer, and fix the code nits. I will then send v2 as a separate
thread per netdev posting rules.

Thanks,
Junnan Zhang