Re: [PATCH net v2] ipv4: fib: treat an unbuildable encapsulation as a nexthop mismatch

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] Incomplete fix: the encapsulation selector of an RTM_DELROUTE is
  still ignored on two reachable paths, so the exact outcome the commit…

--

ipv4: fib: treat an unbuildable encapsulation as a nexthop mismatch

fib_encap_match() used to leave its result at 0 when lwtunnel_build_state()
failed, which fib_nh_match() reads as "the nexthop matches".  The patch
returns a mismatch in that case and drops the now redundant
LWTUNNEL_ENCAP_NONE test, since lwtunnel_build_state() rejects that type
itself.

> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 50e96f86ca59a..001da9fc86cc3 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -911,17 +911,14 @@ static int fib_encap_match(struct net *net, u16 encap_type,
>  			   struct netlink_ext_ack *extack)
>  {
>  	struct lwtunnel_state *lwtstate;
> -	int ret, result = 0;
> +	int result;
>  
> -	if (encap_type == LWTUNNEL_ENCAP_NONE)
> -		return 0;
> +	if (lwtunnel_build_state(net, encap_type, encap, AF_INET, cfg,
> +				 &lwtstate, extack))
> +		return 1;
>  
> -	ret = lwtunnel_build_state(net, encap_type, encap, AF_INET,
> -				   cfg, &lwtstate, extack);
> -	if (!ret) {
> -		result = lwtunnel_cmp_encap(lwtstate, nh->fib_nh_lws);
> -		lwtstate_free(lwtstate);
> -	}
> +	result = lwtunnel_cmp_encap(lwtstate, nh->fib_nh_lws);
> +	lwtstate_free(lwtstate);
>  
>  	return result;
>  }

[Severity: High]
The change to fib_encap_match() looks right on its own, but does it actually
cover the case described in the changelog?

The commit message says:

    So if there comes along a RTM_DELROUTE which carries an encapsulation
    the kernel rejects, it could delete a different route with a different
    encapsulation.

fib_encap_match() has a single caller, and that call is nested inside a test
for an output interface or a gateway in fib_nh_match():

net/ipv4/fib_semantics.c:fib_nh_match() {
	...
	if (cfg->fc_oif || cfg->fc_gw_family) {
		struct fib_nh *nh;

		nh = fib_info_nh(fi, 0);
		if (cfg->fc_encap) {
			if (fib_encap_match(net, cfg->fc_encap_type,
					    cfg->fc_encap, nh, cfg, extack))
				return 1;
		}
	...
}

rtm_to_fib_config() records RTA_ENCAP unconditionally and does not require
RTA_OIF or RTA_GATEWAY next to it:

net/ipv4/fib_frontend.c:rtm_to_fib_config() {
	...
		case RTA_ENCAP:
			cfg->fc_encap = attr;
			break;
	...
}

So for an RTM_DELROUTE carrying only RTA_DST plus RTA_ENCAP (and no
RTA_OIF, no RTA_GATEWAY, no RTA_MULTIPATH), isn't the whole block above
skipped, leaving fib_nh_match() to fall through to:

net/ipv4/fib_semantics.c:fib_nh_match() {
	...
#ifdef CONFIG_IP_ROUTE_MULTIPATH
	if (!cfg->fc_mp)
		return 0;
	...
}

and fib_table_delete() then picks that alias?

net/ipv4/fib_trie.c:fib_table_delete() {
	...
		    fib_nh_match(net, cfg, fi, extack) == 0 &&
		    fib_metrics_match(cfg, fi)) {
			fa_to_delete = fa;
	...
}

Two aliases with different lwtunnel encapsulation can coexist, since
nh_comp() distinguishes them via lwtunnel_cmp_encap(), for example:

  ip route append 10.0.0.0/24 encap ip id 1 dst A dev eth0
  ip route append 10.0.0.0/24 encap ip id 2 dst B dev eth0

A delete request for 10.0.0.0/24 carrying the id 2 encapsulation, or an
RTA_ENCAP with no RTA_ENCAP_TYPE (encap_type == LWTUNNEL_ENCAP_NONE, which
lwtunnel_build_state() rejects with -EINVAL), and no oif or gateway, would
then remove the id 1 route instead.  Should the encap comparison be moved
out from under the cfg->fc_oif || cfg->fc_gw_family test so this path is
covered too?

A second path looks similar.  The RTA_MULTIPATH loop in fib_nh_match()
compares rtnh_ifindex, RTA_GATEWAY/RTA_VIA and, with
CONFIG_IP_ROUTE_CLASSID, RTA_FLOW, but never the nested RTA_ENCAP and
RTA_ENCAP_TYPE:

net/ipv4/fib_semantics.c:fib_nh_match() {
	...
	for_nexthops(fi) {
		int attrlen;

		if (!rtnh_ok(rtnh, remaining))
			return -EINVAL;

		if (rtnh->rtnh_ifindex && rtnh->rtnh_ifindex != nh->fib_nh_oif)
			return 1;
	...
}

The add side does consume those attributes in fib_get_nhs():

net/ipv4/fib_semantics.c:fib_get_nhs() {
	...
			fib_cfg.fc_encap = nla_find(attrs, attrlen, RTA_ENCAP);
	...
}

and rtm_to_fib_config() only validates the nested encapsulation type
number, never the payload:

net/ipv4/fib_frontend.c:rtm_to_fib_config() {
	...
		case RTA_MULTIPATH:
			err = lwtunnel_valid_encap_type_attr(nla_data(attr),
							     nla_len(attr),
							     extack);
	...
}

Does this make ECMP routes that differ only in per-nexthop encapsulation
interchangeable for deletion?

If relocating the check and teaching the multipath loop about RTA_ENCAP is
out of scope here, could the changelog be narrowed to say the fix applies
only to requests that also carry an oif or a gateway, so backporters do not
read it as closing the whole case?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921130736.210845-1-g.goller%40proxmox.com