Re: [PATCH net-next 02/15] netfilter: flowtable: fix pmtu discovery in nf_flow_tunnel_ip6ip6_push()

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

On Sat, Nov 14, 2026 at 03:23:39AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The fix does nothing.
> - [Low] The commit subject and body name nf_flow_tunnel_ip6ip6_push(),
>   which does not exist in the tree being patched.
> 
> --
> 
> netfilter: flowtable: fix pmtu discovery in nf_flow_tunnel_ip6ip6_push()
> 
> This commit moves skb_dst_update_pmtu_no_confirm() ahead of
> skb_scrub_packet() in nf_flow_tunnel_ip6_push(). The aim is to run the
> PMTU update before skb_scrub_packet() drops the skb dst. The inner route
> would then get the tunnel MTU, as it does in ip6_tnl_xmit().
> 
> > netfilter: flowtable: fix pmtu discovery in nf_flow_tunnel_ip6ip6_push()
> >
> > nf_flow_tunnel_ip6ip6_push() invokes skb_scrub_packet() before updating
> > the PMTU.
> 
> [Severity: Low]
> This isn't a bug, but there is no nf_flow_tunnel_ip6ip6_push() in this
> tree. Commit 93cf357fa797 added it under that name, and it was later
> renamed to nf_flow_tunnel_ip6_push(). It now also takes a dst argument and
> handles an IPPROTO_IPIP inner protocol.
> 
> Could the subject and body use the current name? It could also say that
> the change touches 4in6 encapsulation (e.g. DS-Lite) as well as IP6IP6.
> 
> > diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> > index 19cafae748abe..644e70fd6773b 100644
> > --- a/net/netfilter/nf_flow_table_ip.c
> > +++ b/net/netfilter/nf_flow_table_ip.c
> > @@ -699,10 +699,10 @@ static int nf_flow_tunnel_ip6_push(struct net *net, struct sk_buff *skb,
> >  	if (err)
> >  		return err;
> >  
> > -	skb_scrub_packet(skb, true);
> >  	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);
> 
> [Severity: High]
> On the flowtable fast path, is there a dst on the skb at this point?

This should call:

  dst->ops->update_pmtu(dst, NULL, skb, mtu, false);

instead given that skb_dst() is attached later.

The scrub ordering is wrong, but the dst is not attached to the
skbuff.

> Flowtable hooks can only be registered at netdev ingress:
> 
> net/netfilter/nf_tables_api.c:nft_flowtable_parse_hook() {
>     ...
> 	if (hooknum != NF_NETDEV_INGRESS)
> 		return -EOPNOTSUPP;
>     ...
> }
> 
> This means nf_flow_offload_ipv6_hook() runs before ip6_route_input(), so
> no routing dst is attached yet.
>
> The outer tunnel dst only comes in as the
> dst argument:
> 
> nf_flow_offload_ipv6_hook()
>   nf_flow_queue_xmit6()
>     nf_flow_tunnel_push(state->net, skb, other_tuple,
>                         tuplehash->tuple.dst_cache, &ip_addr)
>       nf_flow_tunnel_ip6_push()
> 
> The skb only gets that dst after the push returns, and only for
> FLOW_OFFLOAD_XMIT_NEIGH:
> 
> net/netfilter/nf_flow_table_ip.c:nf_flow_queue_xmit6() {
>     ...
> 		xmit.dest = neigh->ha;
> 		skb_dst_drop(skb);
> 		skb_dst_set_noref(skb, &rt->dst);
> 		break;
> 	case FLOW_OFFLOAD_XMIT_DIRECT:
>     ...
> }
> 
> skb_dst_update_pmtu_no_confirm() does nothing when skb_dst(skb) is NULL:
> 
> include/net/dst.h:skb_dst_update_pmtu_no_confirm() {
> 	struct dst_entry *dst = skb_dst(skb);
> 
> 	if (dst && dst->ops->update_pmtu)
> 		dst->ops->update_pmtu(dst, NULL, skb, mtu, false);
> }
> 
> So the mtu computed from dst_mtu(dst) looks like it is thrown away both
> before and after this change.
> 
> Ingress sometimes attaches a metadata dst (collect_md or tunnel_key).
> That dst uses dst_blackhole_ops, and dst_blackhole_update_pmtu() is
> empty. It is also not the flow's inner route.
> 
> There also doesn't seem to be an inner route left to update, because
> nft_dev_forward_path() replaces it with the tunnel dst:
> 
> net/netfilter/nf_flow_table_path.c:nft_dev_forward_path() {
>     ...
> 	dst_release(route->tuple[dir].dst);
> 	route->tuple[dir].dst = info.tun_dst;
>     ...
> }
> 
> The ip6_tnl_xmit() comparison depends on ip6_forward() having already put
> the inner route on the skb. The flowtable fast path skips that step.
> 
> If that reading is right, the patch reorders a call that never has any
> effect. The PMTU update described as "silently lost" would still be lost
> on every fast-path packet.
> 
> Would one of these work better: dropping the call, updating an
> explicitly stored inner dst, or relying on (and documenting) the
> nf_flow_exceeds_mtu() check in nf_flow_offload_ipv6_forward()? That check
> already sends oversized packets to the slow path, and ip6_tnl_xmit()
> handles PMTU there.
> 
> At the end of the series the code still does the skb_dst(skb) based
> PMTU update, followed by skb_scrub_packet(skb, false), so later patches
> don't seem to fix this either.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org