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

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

[Junnan Zhang]

From: Junnan Zhang <zhangjn11@chinatelecom.cn>

AF_PACKET SOCK_RAW reserves dev->hard_header_len bytes of headroom. For
VLAN subinterfaces, hard_header_len includes VLAN tag space (18 bytes)
while min_header_len is the real Ethernet header length (14 bytes). When
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.

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@chinatelecom.cn>
Signed-off-by: Shouxin Sun <sunshx@chinatelecom.cn>
Signed-off-by: Junnan Zhang <zhangjn_dev@163.com>
---
 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;
 
 	/* 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) &&
+	    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

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@chinatelecom.cn>
> 
> 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@chinatelecom.cn>
> Signed-off-by: Shouxin Sun <sunshx@chinatelecom.cn>
> Signed-off-by: Junnan Zhang <zhangjn_dev@163.com>
> ---
>  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
> 

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

[Junnan Zhang]

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