Re: [PATCH net-next 11/15] netfilter: flowtable: do not scrub as crossing netns

[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 Fixes: tag and the commit message cover only one of the two
  changed call sites.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch.

--

netfilter: flowtable: do not scrub as crossing netns

This commit changes the flowtable tunnel push helpers from
skb_scrub_packet(skb, true) to skb_scrub_packet(skb, false). The skb mark
is now kept on encapsulated packets. The commit message says the IPIP
tunneling fast path does not cross a netns.

> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 644e70fd6773b..001abfae5a1ff 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -630,7 +630,7 @@ static int nf_flow_tunnel_ip_push(struct net *net, struct sk_buff *skb,
>  	if (err)
>  		return err;
>  
> -	skb_scrub_packet(skb, true);
> +	skb_scrub_packet(skb, false);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Is
the hard-coded xnet = false in nf_flow_tunnel_ip_push() and
nf_flow_tunnel_ip6_push() correct only because the fast path routes in
dev_net(dev) and not in the tunnel's link netns?

The slow path routes in tunnel->net or t->net, and it decides xnet per
device:

net/ipv4/ip_tunnel.c:ip_tunnel_xmit() {
    ...
	iptunnel_xmit(NULL, rt, skb, fl4.saddr, fl4.daddr, proto, tos, ttl,
		      df, !net_eq(tunnel->net, dev_net(dev)), 0);
    ...
}

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit() {
    ...
	skb_scrub_packet(skb, !net_eq(t->net, dev_net(dev)));
    ...
}

The forward path helpers that admit the flow look up the outer route in
the device netns instead:

net/ipv4/ipip.c:ipip_fill_forward_path() {
    ...
	rt = ip_route_output(dev_net(ctx->dev), tiph->daddr, tiph->saddr,
			     inet_dsfield_to_dscp(tiph->tos),
			     tunnel->parms.link, RT_SCOPE_UNIVERSE);
    ...
}

net/ipv6/ip6_tunnel.c:ip6_tnl_fill_forward_path() {
    ...
	dst = ip6_route_output(dev_net(ctx->dev), NULL, &fl6);
    ...
}

Neither helper rejects a tunnel whose link netns differs from
dev_net(dev). nft_dev_path_info() does not reject it either, because it
only checks nft_flowtable_find_dev(info->dev, ft).

Take an ipip or ip6tnl device whose link netns is A but which lives in
netns B. Could the offloaded packets of a flow be routed with B's routing
table, while the slow-path packets of the same flow leave through A's
underlay?

With this patch the two paths would also handle skb->mark differently.
The slow path clears it and the fast path keeps it.

Could ipip_fill_forward_path() and ip6_tnl_fill_forward_path() return
-EOPNOTSUPP when the tunnel's link netns differs from dev_net(dev)? That
would also keep xnet = false correct. Another option is to route in
tunnel->net and carry the real xnet value through flow_offload_tunnel.

> @@ -702,7 +702,7 @@ static int nf_flow_tunnel_ip6_push(struct net *net, struct sk_buff *skb,
>  	mtu = dst_mtu(dst) - sizeof(*ip6h);
>  	mtu = max(mtu, IPV6_MIN_MTU);
>  	skb_dst_update_pmtu_no_confirm(skb, mtu);
> -	skb_scrub_packet(skb, true);
> +	skb_scrub_packet(skb, false);

[Severity: Low]
This isn't a bug, but the skb_scrub_packet() call in
nf_flow_tunnel_ip6_push() came from 93cf357fa797 ("netfilter: flowtable:
Add IP6IP6 tx sw acceleration"). It did not come from the commit named
in the tag:

  Fixes: d30301ba4b07 ("netfilter: flowtable: Add IPIP tx sw acceleration")

The commit message also only talks about "IPIP tunneling support".
However, this hunk changes the IPv6-outer encapsulation path too, so
skb->mark is now kept on those packets as well.

Earlier in the same series, "netfilter: flowtable: fix pmtu discovery in
nf_flow_tunnel_ip6ip6_push()" uses Fixes: 93cf357fa797 for this same
function.

Should a Fixes: tag for 93cf357fa797 be added, and should the commit
message mention the IPv6-outer path?

[ ... ]

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