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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sat, Nov 14, 2026 at 03:23:49AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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).

Not sure this is correct, if you move a netdevice in a flowtable to
another netns, it gets removed from the flowtable.

> 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