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

[Willem de Bruijn]

Junnan Zhang wrote:
> From: Junnan Zhang <zhangjn11@xxxxxxxxxxxxxxx>
> 
> AF_PACKET SOCK_RAW reserves dev->hard_header_len bytes of headroom. For
> VLAN subinterfaces, hard_header_len includes VLAN tag space (18 bytes)

Does it?

vlan_dev_init:

        dev->hard_header_len = real_dev->hard_header_len;

> while min_header_len is the real Ethernet header length (14 bytes). When

Which device did you observe this with?

> userspace sends a standard untagged Ethernet frame through a VLAN
> subinterface, 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 4 bytes too late and
> inet_gso_segment() fails with -EINVAL.

Which path did you observe generating these untagged packets through
a VLAN interface?

> 
> 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>
> ---
>  net/packet/af_packet.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 1168bd6b09cd..4669320f551b 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 has_vlan;

nit: confusing variable, combining test on packet and device.
>  
>  	/* On TX skb->data is the L2 header; anchor it for all socket types. */
>  	skb_reset_mac_header(skb);
> @@ -1943,11 +1944,23 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  	    sock->type == SOCK_RAW)
>  		skb->protocol = dev_parse_header_protocol(skb);
>  
> +	has_vlan = likely(skb->dev->type == ARPHRD_ETHER) &&
> +		   eth_type_vlan(skb->protocol);
> +
> +	/* For non-VLAN raw frames on devices whose hard_header_len includes
> +	 * VLAN tag space (e.g. VLAN subinterfaces), the network header must be
> +	 * at the actual L2/L3 boundary, not hard_header_len, so that both the
> +	 * transport header probe below and subsequent GSO see the right L3.
> +	 */
> +	if (!has_vlan && sock->type == SOCK_RAW &&
> +	    likely(skb->dev->type == ARPHRD_ETHER) &&

nit: repeat test, also included in that has_vlan

> +	    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);
>  
>  	/* Move network header to the right position for VLAN tagged packets */
> -	if (likely(skb->dev->type == ARPHRD_ETHER) &&
> -	    eth_type_vlan(skb->protocol) &&
> +	if (has_vlan &&
>  	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
>  		skb_set_network_header(skb, depth);
>  }
> -- 
> 2.43.0
>