Re: [PATCH net v2] net/packet: fix network header offset for non-VLAN raw packets

[Willem de Bruijn]

Junnan Zhang wrote:
> AF_PACKET SOCK_RAW sets skb network_header to dev->hard_header_len in
> packet_snd(). For Ethernet devices whose hard_header_len exceeds the
> on-wire L2 header length (min_header_len = ETH_HLEN) -- e.g.
> software-offload VLAN subinterfaces, where hard_header_len = ETH_HLEN +
> VLAN_HLEN = 18 -- a non-VLAN SOCK_RAW frame still carries a standard
> 14-byte Ethernet header, so its L3 header sits at min_header_len, not
> hard_header_len.
> 
> packet_parse_headers() only corrects network_header for VLAN-tagged
> frames. For non-VLAN frames it leaves network_header at hard_header_len,
> so the IP header is found (hard_header_len - min_header_len) bytes too
> late and inet_gso_segment() fails with -EINVAL.
> 
> Observed on a virtio_net NIC (KVM guest) that advertises
> NETIF_F_HW_VLAN_CTAG_FILTER but not NETIF_F_HW_VLAN_CTAG_TX, so VLAN
> subinterfaces use software tag insertion (hard_header_len = 18). An
> AF_PACKET SOCK_RAW socket bound to the VLAN subinterface with
> PACKET_VNET_HDR enabled sends a large IPv4/TCP frame exceeding the path
> MTU, with gso_type set in the virtio-net header. The user frame is a
> plain [ethhdr][IP...] layout without a VLAN tag; the VLAN subdevice
> inserts the 802.1Q tag in vlan_dev_hard_start_xmit(). 

Where does it do this? validate_xmit_vlan does this on the physical
device I think.

That adds a bigger problem that the extra needed headroom of
(hard_header_len - min_header_len) is not reserved at the start of the
frame.

In packet_snd, reserve == hard_header_len, so the packet socket
starts writing at the start of the buffer. When
__vlan_insert_inner_tag inserts the tag, it tries to move the mac
header back, leaving the network header in place. Which works fine if
the network header was set correctly from the start, and the original
mac started at network_header - min_header_len.

This is all messy because before needed_headroom existed, legacy
tunnels had to resort to increasing hard_header_len to reserve extra
room for their outer headers.

Long aside, and I don't expect to address/disentangle that in this bug
fix targeting net. Will take a look whether we can safely do that in
net-next.

But please Link to this version of the patch (and thus the thread)
when sending the next version.

> With
> network_header stuck at 18 while the real IP header is at ETH_HLEN (14),
> inet_gso_segment() reads a misaligned ip_hdr(skb) and returns -EINVAL.
> 
> Set network_header to min_header_len for non-VLAN SOCK_RAW frames on
> Ethernet devices whose hard_header_len exceeds min_header_len, so the
> L3/L4 header positions match the actual on-the-wire frame.
> 
> This fix is placed before skb_probe_transport_header() so that both the
> transport header probe (which uses skb_network_offset() as nhoff) and
> subsequent GSO see the right L3/L4 offsets. It complements
> commit 01fdecc0480d ("net: packet: fix wrong transport_header when sending VLAN-tagged frame")
> which only covers VLAN-tagged frames.
> 
> Fixes: dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")
> Signed-off-by: Junnan Zhang <zhangjn11@xxxxxxxxxxxxxxx>
> Signed-off-by: Shouxin Sun <sunshx@xxxxxxxxxxxxxxx>
> Signed-off-by: Junnan Zhang <zhangjn_dev@xxxxxxx>
> ---
> v2:
> - Drop "on VLAN subinterfaces" from the subject and reword the scope to
>   Ethernet devices whose hard_header_len exceeds the on-wire L2 header
>   length (min_header_len). The min_header_len < hard_header_len test is
>   not VLAN-specific; it also covers Ethernet drivers that reserve extra
>   hard_header_len space for driver-internal wrapping.
> - Restructure packet_parse_headers() to test dev->type once; replace
>   has_vlan, which mixed the device and packet tests, with the
>   packet-only is_vlan.
> - Describe the header offset error generically as (hard_header_len -
>   min_header_len) bytes instead of hard-coding the 4-byte VLAN case.
> - Document the reproducer: virtio_net advertising
>   NETIF_F_HW_VLAN_CTAG_FILTER but not NETIF_F_HW_VLAN_CTAG_TX, with
>   PACKET_VNET_HDR and GSO triggering the -EINVAL from
>   inet_gso_segment().
> 
> v1: https://lore.kernel.org/all/20260821085722.24036-1-zhangjn_dev@xxxxxxx/#t
> ---
>  net/packet/af_packet.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 1168bd6b09cd..be5bf9db7ea1 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1935,6 +1935,7 @@ 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;
> +	bool is_vlan = false;
>  
>  	/* On TX skb->data is the L2 header; anchor it for all socket types. */
>  	skb_reset_mac_header(skb);
> @@ -1943,11 +1944,28 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  	    sock->type == SOCK_RAW)
>  		skb->protocol = dev_parse_header_protocol(skb);
>  
> +	if (likely(skb->dev->type == ARPHRD_ETHER)) {
> +		is_vlan = eth_type_vlan(skb->protocol);
> +
> +		/* For non-VLAN SOCK_RAW frames on Ethernet devices whose
> +		 * hard_header_len exceeds the on-wire L2 header length
> +		 * (min_header_len) -- e.g. software-offload VLAN subinterfaces,
> +		 * or Ethernet drivers that reserve extra space in
> +		 * hard_header_len for driver-internal wrapping -- the SOCK_RAW
> +		 * send paths leave network_header at hard_header_len, while the
> +		 * user frame's L3 sits at min_header_len. Move network_header
> +		 * to the actual L2/L3 boundary so the transport header probe
> +		 * below and subsequent GSO see the right L3.
> +		 */
> +		if (!is_vlan && sock->type == SOCK_RAW &&
> +		    skb->dev->min_header_len < skb->dev->hard_header_len)

Instead of using min_header_len != hard_header_len to detect vlan devices,
consider is_vlan_dev(). That does not have false positives for other
protocols, and makes the code more self-descriptive.

Unless we are certain that the same also exhibits for other devices that
play hard_header_len games, such as GRE tunnels. But then the
ARPHRD_ETHER check excludes those anyway. So for now I would focus on the
VLAN issue only.

  if (sock->type == SOCK_RAW && !is_vlan_packet && is_vlan_dev(skb->dev))
    skb_set_network_header(skb, skb->dev->min_header_len);

It sucks that we have to add another branch in the hot path for an edge
case. Would be preferable if we can fix this in the vlan driver. But
that will come too late for skb_probe_transport_header.

> +			skb_set_network_header(skb, skb->dev->min_header_len);
> +	}
> +
>  	skb_probe_transport_header(skb);
>  
>  	/* Move network header to the right position for VLAN tagged packets */
> -	if (likely(skb->dev->type == ARPHRD_ETHER) &&
> -	    eth_type_vlan(skb->protocol) &&
> +	if (is_vlan &&
>  	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
>  		skb_set_network_header(skb, depth);
>  }
> -- 
> 2.43.0
>