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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] The new branch added to packet_parse_headers()
  (net/packet/af_packet.c:1950-1966) is a no-op in exactly the…
- [Medium] The only configurations in which the new statement in
  packet_parse_headers() (net/packet/af_packet.c:1963-1965) changes any…

--

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

This adds a branch to packet_parse_headers() that, for untagged SOCK_RAW
frames sent on a VLAN subinterface, moves skb->network_header to
dev->min_header_len before skb_probe_transport_header() runs.  The stated
goal is to make GSO see the L3 header at ETH_HLEN instead of at
hard_header_len.  The existing VLAN-tagged correction below is reworked to
reuse the newly computed is_vlan_packet boolean.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 76bde7906d494..8dfb823e819d6 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1938,6 +1938,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_packet = false;
>  
>  	/* On TX skb->data is the L2 header; anchor it for all socket types. */
>  	skb_reset_mac_header(skb);
> @@ -1946,11 +1947,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_packet = eth_type_vlan(skb->protocol);
> +
> +		/* For non-VLAN SOCK_RAW frames on VLAN subinterfaces with
> +		 * software tag insertion, hard_header_len includes space
> +		 * for the VLAN tag while min_header_len is the on-wire
> +		 * Ethernet header length. The user frame carries a
> +		 * standard Ethernet header, so its L3 sits at
> +		 * min_header_len, not hard_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 (sock->type == SOCK_RAW && !is_vlan_packet &&
> +		    is_vlan_dev(skb->dev))
> +			skb_set_network_header(skb, skb->dev->min_header_len);
> +	}

[Severity: High]
Is this new assignment a no-op in exactly the configuration the changelog
describes?

The commit message says:

  "On VLAN subinterfaces with software tag insertion, hard_header_len
   includes space for the VLAN tag (ETH_HLEN + VLAN_HLEN = 18) while
   min_header_len is the on-wire Ethernet header length (ETH_HLEN = 14)."

That does not appear to hold in this tree any more.  vlan_dev_init() copies
the real device's value verbatim and accounts the tag space in
needed_headroom instead:

net/8021q/vlan_dev.c:vlan_dev_init() {
	dev->needed_headroom = real_dev->needed_headroom + VLAN_HLEN;
	dev->needed_tailroom = real_dev->needed_tailroom;
	dev->header_ops      = &vlan_header_ops;
	dev->hard_header_len = real_dev->hard_header_len;
}

vlan_transfer_features() does the same, so toggling
NETIF_F_HW_VLAN_CTAG_TX no longer changes hard_header_len at runtime
either.  This came in with commit 447cbe95ebb953 ("vlan: fix
skb_under_panic and races when toggling HW VLAN offload"), which is already
an ancestor of the baseline, and whose changelog states "AF_PACKET SOCK_RAW
network header offsets remain correctly aligned at
real_dev->hard_header_len".

min_header_len is never touched by net/8021q; vlan_setup() goes through
ether_setup():

net/ethernet/eth.c:ether_setup() {
	dev->hard_header_len 	= ETH_HLEN;
	dev->min_header_len	= ETH_HLEN;
}

So for a VLAN device over an ordinary Ethernet NIC, including the reported
virtio_net guest, hard_header_len == min_header_len == 14, and packet_snd()
has already placed network_header at mac_header + 14:

net/packet/af_packet.c:packet_snd() {
	skb_reset_network_header(skb);
	...
	} else if (reserve) {
		skb_reserve(skb, -reserve);
		if (len < reserve + sizeof(struct ipv6hdr) &&
		    dev->min_header_len != hard_header_len)
			skb_reset_network_header(skb);
	}
}

tpacket_fill_skb() reaches the same offset via skb_reset_network_header()
followed by skb_push(skb, hard_header_len).

If hard_header_len is 14 there, does skb_set_network_header(skb,
min_header_len) write back the identical offset, leaving the reported
inet_gso_segment() -EINVAL unchanged?  If the failure is real, would it be
worth re-deriving it against 447cbe95ebb953 and keying the fix off the
frame's actual content rather than off device header-length metadata?

The in-code comment and the Fixes: tag also describe device geometry that
447cbe95ebb953 removed, and would propagate that description into stable
trees.

[Severity: Medium]
The only stacks where this statement changes a value are VLAN devices whose
real device has hard_header_len != ETH_HLEN.  Does it then conflict with the
deliberate reset in packet_snd()?

The short-frame safeguard there is gated on precisely the same inequality
that makes the new code effective:

net/packet/af_packet.c:packet_snd() {
		if (len < reserve + sizeof(struct ipv6hdr) &&
		    dev->min_header_len != hard_header_len)
			skb_reset_network_header(skb);
}

That reset came from commits 993675a3100b16 and 88a8121dc1d3d0.  For a VLAN
created over, say, drivers/net/usb/dm9601.c (dm9601_bind() does
dev->net->hard_header_len += DM_TX_OVERHEAD), any SOCK_RAW frame with
14 <= len < hard_header_len + sizeof(struct ipv6hdr) gets network_header
reset to the MAC header by packet_snd(), and then moved back to
mac_header + 14 by the new statement a few instructions later.  Which of the
two sites is meant to win?

There is also the reverse case.  cx82310_eth is ARPHRD_ETHER, does not set
NETIF_F_VLAN_CHALLENGED, and zeroes hard_header_len:

drivers/net/usb/cx82310_eth.c:cx82310_bind() {
	dev->net->hard_header_len = 0;
}

vlan_check_real_dev() only rejects NETIF_F_VLAN_CHALLENGED and non
ARPHRD_ETHER devices, so a VLAN device over it inherits hard_header_len 0
while min_header_len stays ETH_HLEN.  dev_validate_header() then accepts a
SOCK_RAW frame shorter than 14 bytes.  For such a frame, does
skb_set_network_header(skb, min_header_len) place network_header past
skb->tail, so that a later skb_reset_mac_len() (net/core/gso.c and
net/core/dev.c) computes skb->mac_len = 14 with skb->len below that?  That
looks like the state 993675a3100b16 was written to avoid.

Would gating on the frame contents, or on the actual position of the L2/L3
boundary in the skb, avoid touching these unrelated device stacks?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901074214.57717-1-zhangjn_dev%40163.com