[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

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

[Willem de Bruijn]

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) &&
> >

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

[Junnan Zhang]

Hi Willem,
Thanks for the follow-up.
> Why is the real length 14 + 14 == 28?
> Where does the second 14 come from?
You're right to flag this - my previous mail used "base" inconsistently,
which is what made the "14 + 14" look unexplained. Let me redo it with a
single reference point.
The cleanest reference is skb->data after packet_snd() has set things up
(i.e. the start of the user-supplied raw frame). With hard_header_len=18
and min_header_len=14 on a software-offload VLAN subif:
  - skb_reset_network_header() runs while data is at head + hlen
    (hlen = LL_RESERVED_SPACE_EX(dev, 18) = 32, the HH_DATA_MOD-rounded
     headroom), so network_header lands at head + hlen = head + 32.
  - The SOCK_RAW branch then does skb_reserve(skb, -reserve) with
    reserve = hard_header_len = 18, moving data back to head + 14
    (= head + hlen - hard_header_len).
  - The small-frame skb_reset_network_header() at packet_snd:3078 does
    not fire for a GSO frame, so network_header stays at head + 32.
Relative to data (= head + 14), this means:
    network_header = data + (32 - 14) = data + 18 = data + hard_header_len
    real IP header  = data + ETH_HLEN  = data + 14 = data + min_header_len
So network_header points VLAN_HLEN (4) bytes past the real IP header.
The "14 + 14 = 28" in my earlier reply was the absolute offset of IP
from head (data-offset 14 from rounding + ETH_HLEN 14); the second 14 is
ETH_HLEN, i.e. the user-supplied Ethernet header. I should not have mixed
the head-relative IP position with a data-relative network_header - sorry
for the confusion. v2 will use data-relative offsets throughout, which
also matches skb_network_offset() and is independent of the
LL_RESERVED_SPACE rounding.
With the fix, skb_set_network_header(skb, dev->min_header_len) sets
network_header = data + min_header_len = data + ETH_HLEN, i.e. exactly on
the real IP header, so both skb_probe_transport_header() (nhoff) and GSO
see the correct L3.

> 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.
>
You're right, it is not specific to VLAN. min_header_len <
hard_header_len also matches Ethernet drivers that reserve extra space
in hard_header_len beyond ETH_HLEN for their own wrapping. For all of these, 
the user-supplied non-VLAN SOCK_RAW frame still carries a standard 14-byte 
Ethernet header, so its L3 header sits at ETH_HLEN = min_header_len, and 
pointing network_header there is correct.
The condition is intentionally generic, not VLAN-specific.
For any ARPHRD_ETHER device whose hard_header_len exceeds min_header_len,
a non-VLAN SOCK_RAW frame's L3 sits at min_header_len (the standard
Ethernet header length), regardless of what extra bytes hard_header_len
reserves for driver-internal wrapping. The fix points network_header at
exactly that L2/L3 boundary. This matches the existing
dev->min_header_len != hard_header_len check already used in packet_snd()
(the small-frame skb_reset_network_header path).
I'll reword the v2 commit message to describe the scope as "Ethernet
devices whose hard_header_len exceeds min_header_len" rather than "VLAN
subinterfaces", and drop the misleading VLAN-only framing.
I'll send v2 as a new thread.

Thanks,
Junnan