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

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.

[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