Re: [PATCH net 1/2] ipv6: honor rule suppression in raw FIB lookups

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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).

- [Medium] For raw lookups, fib6_rule_suppress() (net/ipv6/fib6_rules.c)
  evaluates suppress_ifgroup against a different nexthop than the dst-…

--

ipv6: honor rule suppression in raw FIB lookups

This patch makes fib6_rule_suppress() apply suppress_prefixlength and
suppress_ifgroup to raw fib6_table_lookup() results. For those results it
takes the prefix length from res->f6i and the device from res->nh. Raw
results marked RTF_REJECT are never suppressed, so a later rule cannot
bypass the rejection.

Link: https://github.com/firewalld/firewalld/issues/1600

> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index 0ffd7b47a0460..e8a44b540291d 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -283,19 +283,29 @@ INDIRECT_CALLABLE_SCOPE bool fib6_rule_suppress(struct fib_rule *rule,
>  						struct fib_lookup_arg *arg)
>  {
>  	struct fib6_result *res = arg->result;
> -	struct rt6_info *rt = res->rt6;
> +	struct rt6_info *rt = NULL;
>  	struct net_device *dev = NULL;
> -
> -	if (!rt)
> -		return false;
> -
> -	if (rt->rt6i_idev)
> -		dev = rt->rt6i_idev->dev;
> +	int plen;
> +
> +	if (arg->lookup_ptr == fib6_table_lookup) {
> +		/* Raw lookups report reject routes without an error. */
> +		if (res->fib6_flags & RTF_REJECT)
> +			return false;
> +		plen = res->f6i->fib6_dst.plen;
> +		dev = res->nh->fib_nh_dev;

[Severity: Medium]
For multipath routes, is this the same nexthop that the dst-based path
checks?

On the raw path, res->nh is whatever fib6_table_lookup() picked. For
multipath that is usually the first sibling or the first usable member of
the nexthop group. fib6_rule_action_alt() never calls fib6_select_path().

The dst-based path selects the path before it builds the rt6_info whose
rt6i_idev the old code looked at:

net/ipv6/route.c:ip6_pol_route() {
    ...
	fib6_select_path(net, &res, fl6, oif, have_oif_match, skb, strict);
    ...
}

fib6_select_path() then replaces res->nh (and res->f6i for siblings) with
the hash-selected path, which may be on a different device:

net/ipv6/route.c:fib6_select_path() {
    ...
	if (unlikely(match->nh)) {
		nexthop_path_fib6_result(res, fl6->mp_hash);
		return;
	}
    ...
out:
	res->f6i = match;
	res->nh = match->fib6_nh;
}

bpf_ipv6_fib_lookup() runs path selection only after fib6_lookup() has
already made the suppression decision:

net/core/filter.c:bpf_ipv6_fib_lookup() {
    ...
	err = fib6_lookup(net, oif, &fl6, &res, strict);
    ...
	fib6_select_path(net, &res, &fl6, fl6.flowi6_oif,
			 fl6.flowi6_oif != 0, NULL, strict);
    ...
	dev = res.nh->fib_nh_dev;
	params->ifindex = dev->ifindex;
    ...
}

Consider a multipath route whose nexthops are on devices in different
interface groups, plus a rule like "ip -6 rule ... lookup <tbl>
suppress_ifgroup N". Could a flow be suppressed by the ordinary route
lookup but not by the nft fib or BPF raw lookup, or the other way round?

Can bpf_fib_lookup also return a device in the suppressed group that
suppression never evaluated?

That would leave the same raw versus ordinary disagreement the commit
message says this patch removes. suppress_prefixlength does not appear to
be affected, since siblings share one prefix.

> +	} else {
> +		rt = res->rt6;
> +		if (!rt)
> +			return false;
> +		plen = rt->rt6i_dst.plen;
> +		if (rt->rt6i_idev)
> +			dev = rt->rt6i_idev->dev;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110131638.15004-3-leonard%40lausen.nl