Re: [PATCH net-next 12/15] netfilter: flowtable: set inner protocol when popping tunnel header

[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 says the patch makes nf_flow_ip_tunnel_pop()
  pop the outer header and set skb->protocol "instead of bailing out…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  nf_flow_ip_tunnel_pop() strips the outer tunnel header with plain…

--

netfilter: flowtable: set inner protocol when popping tunnel header

This commit makes nf_flow_ip_tunnel_pop() set skb->protocol from
ctx->tun.inner_proto after it pulls the outer tunnel header. The commit
message calls it a preliminary patch for IPv4 over IPv6 and SIT tunnel
flowtable offload.

> Pop the outer tunnel header and set skb->protocol to the inner protocol
> derived from ctx->tun.inner_proto in nf_flow_ip_tunnel_pop(), instead of
> bailing out unless the tunnel carries IP-in-IP or IPv6-in-IPv6.

[Severity: Low]
Is this description accurate? The early return at the top of
nf_flow_ip_tunnel_pop() is unchanged by this patch:

	if (ctx->tun.inner_proto != IPPROTO_IPIP &&
	    ctx->tun.inner_proto != IPPROTO_IPV6)
		return;

The only change is the new skb->protocol assignment.

With the current parsers, that assignment also looks like a no-op:

- nf_flow_ip4_tunnel_proto() sets inner_proto to IPPROTO_IPIP only when
  the outer header is IPv4.
- nf_flow_ip6_tunnel_proto() sets IPPROTO_IPV6 only when the outer header
  is IPv6.
- nf_flow_encap_pop() calls the pop only when skb->protocol is already
  the outer family:

	if (skb->protocol == htons(ETH_P_IP) ||
	    skb->protocol == htons(ETH_P_IPV6))
		nf_flow_ip_tunnel_pop(ctx, skb);

So the value written always matches what skb->protocol already holds.
The rest of the series, up to "netfilter: flowtable: unify tunnel push for
IPv4 and IPv6", does not change the parsers or this guard.

Could the commit message say that the guard is kept? It could also say
that the assignment only has an effect once cross-family parsing
(IPv4-over-IPv6/SIT) is added.

Removing the guard, as the message describes, would not be correct.
Non-tunnel flows have inner_proto == 0, so they would get skb->protocol
set to ETH_P_IPV6.

> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index c8c29a9a16844..29d1a9f4a362b 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -372,6 +372,11 @@ static void nf_flow_ip_tunnel_pop(struct nf_flowtable_ctx *ctx,
>  
>  	skb_pull(skb, ctx->tun.hdr_size);
>  	skb_reset_network_header(skb);
> +
> +	if (ctx->tun.inner_proto == IPPROTO_IPIP)
> +		skb->protocol = htons(ETH_P_IP);
> +	else
> +		skb->protocol = htons(ETH_P_IPV6);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Should
the skb_pull() above be skb_pull_rcsum()?

The VLAN and PPPoE cases in nf_flow_encap_pop() in the same file use
skb_pull_rcsum(). __iptunnel_pull_header() also adjusts the checksum
before it sets the inner protocol:

net/ipv4/ip_tunnel_core.c:__iptunnel_pull_header() {
    ...
	skb_pull_rcsum(skb, hdr_len);
    ...
		skb->protocol = inner_proto;
    ...
}

This patch adds the skb->protocol step from that pattern, but not the
checksum adjustment.

Take a CHECKSUM_COMPLETE skb with an outer IPv6 header (ip6ip6 today,
IPv4-over-IPv6 later). After the pull, skb->csum still includes the 40
bytes of the removed header. An outer IPv4 header does not cause this,
because its valid header checksum makes the header's ones'-complement
sum effectively zero.

Suppose such a forwarded skb is looped back into a receive path. One
example is veth into another netns, where __dev_forward_skb2() keeps
CHECKSUM_COMPLETE. Would the L4 checksum check then fail on the stale
skb->csum? The stack would recompute the checksum in software and call
netdev_rx_csum_fault(), which prints a rate-limited warning.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018005638.1776148-2-pablo%40netfilter.org