Re: [PATCH net v6 2/3] tun: clear stale skb->protocol of NAPI frags skbs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what the bug does in the tree as
  it stands.

--

tun: clear stale skb->protocol of NAPI frags skbs

This commit clears skb->protocol in tun_napi_alloc_frags(). After a
GRO_MERGED_FREE merge, the skb recycled by napi_reuse_skb() still holds
the previous frame's ethertype. The virtio header checks in tun_get_user()
run before eth_type_trans(), so without this change they see that stale
value.

Closes: https://lore.kernel.org/netdev/179386753589.488856.11455181721004759454@kernel.org/

> tun_get_user() calls tun_vnet_hdr_tnl_to_skb() before eth_type_trans(),
> so the virtio header checks see the protocol of the previous frame:
> GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are dissected with the
> wrong protocol. A following change checks the outer protocol of UDP
> tunnel GSO packets, which would then reject a valid frame when the two
> ethertypes differ.

[Severity: Low]
This isn't a bug, but could the commit message say more about the impact
in the current tree? As written, the visible failure seems to depend on
the follow-up change. The stale protocol looks like it already causes
problems without that change.

In __virtio_net_hdr_to_skb(), the GSO branch without NEEDS_CSUM only
parses the real ethertype when skb->protocol is zero:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
	} else if (gso_type) {
		...
		if (!skb->protocol) {
			skb->protocol = dev_parse_header_protocol(skb);
			if (!skb->protocol)
				virtio_net_hdr_set_proto(skb, hdr);
		}
retry:
		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
    ...
}

When napi_frags_skb() has left a non-zero value from the previous frame,
the dissector starts at the network offset and uses the previous frame's
ethertype.

After that, the only check is virtio_net_hdr_match_proto() on the
dissected n_proto. Nothing compares it with the frame's real ethertype.

Two outcomes seem possible today:

  - A valid frame is rejected with -EINVAL. For example, a TCPV4 GSO
    frame arrives after an IPv6 frame was merged. The IPv4 header is then
    parsed as IPv6, and either ip_proto or n_proto fails to match.

  - A GSO frame whose real ethertype does not match its GSO type is
    accepted, because the stale ethertype makes the payload parse as the
    expected protocol. eth_type_trans() then sets an skb->protocol that
    disagrees with gso_type.

Could these effects be mentioned in the message? That would show stable
backporters that the fix matters on its own, not only once the follow-up
change lands.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110213628.3476051-1-edumazet%40kernel.org