Re: [PATCH net-next 07/15] netfilter: nft_flow_offload: do not offload multicast

[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 of "netfilter: nft_flow_offload: do not
  offload multicast" describes a bug that users can see, but the patch…

--

netfilter: nft_flow_offload: do not offload multicast

This patch makes nft_flow_offload_skip() skip packets with a multicast
destination for both IPv4 and IPv6. The flowtable then no longer depends
on the ruleset or on the reverse route lookup to keep multicast flows
out.

> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index d3c5651dd6997..530e20c7fd457 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c

[ ... ]

> @@ -33,6 +34,10 @@ static bool nft_flow_offload_skip(struct sk_buff *skb, int family)
>  
>  		if (unlikely(opt->optlen))
>  			return true;
> +
> +		return ipv4_is_multicast(ip_hdr(skb)->daddr);
> +	} else if (family == NFPROTO_IPV6) {
> +		return ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr);

[Severity: Low]
The commit message describes a bug that users can see. IPv6 multicast
forwarded by ip6mr is added to the flowtable, and the flow keeps sending
to the output it resolved even after the last listener leaves. Should
this have a Fixes: tag, and maybe Cc: stable as well?

The path to the bug looks like this:

ip6mr_forward2()
  NF_HOOK(NFPROTO_IPV6, NF_INET_FORWARD, ...)
    nft_flow_offload_eval()
      nft_flow_route()
      flow_offload_add()

In nft_flow_route() in net/netfilter/nf_flow_table_path.c, the reverse
lookup does:

    fl.u.ip6.saddr = ct->tuplehash[!dir].tuple.src.u3.in6;

For a multicast flow in the original direction, that address is the
multicast group. On IPv4, ip_route_output_key_hash_rcu() returns -EINVAL
for a multicast saddr. The IPv6 output lookup has no such check, so
other_dst resolves and the flow is added.

NF_INET_FORWARD is the only hook nft_flow_offload_validate() accepts. A
ruleset with a plain 'flow add @ft' in a forward chain reaches this path
on current kernels.

Without a Fixes: tag, stable and distro backporters have nothing that
points them at this fix. Older kernels would likely keep sending IPv6
multicast to stale outputs.

>  	}
>  
>  	return false;

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