Re: [PATCH nf] netfilter: ip6t_rpfilter: handle routes without inet6_dev

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

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

- [Medium] net/ipv6/netfilter/ip6t_rpfilter.c:rpfilter_lookup_reverse6()
  now makes strict-mode matching depend on the presence of an optional…

--

netfilter: ip6t_rpfilter: handle routes without inet6_dev

This reorders rpfilter_lookup_reverse6() so loose mode is evaluated before
touching rt->rt6i_idev, and adds a NULL check on rt6i_idev before the strict
mode device comparison.  The stated trigger is a route using an external
nexthop object whose device lost its inet6_dev.

Closes: https://lore.kernel.org/all/VtWUkE8QzJt5CroTj2V2v3ZQ0gwbXZ7nq7I3@bugs.sh/ (local)

[Severity: Medium]
Should the strict-mode comparison use the route's nexthop device instead of
rt6i_idev?  With this form, strict mode returns no-match for exactly the
routes the commit message describes, i.e. those whose device has no
inet6_dev, even when the packet did arrive on that device.

rt6i_idev is filled in best-effort in ip6_rt_copy_init():

net/ipv6/route.c:ip6_rt_copy_init() {
	...
	rt->rt6i_idev = dev ? in6_dev_get(dev) : NULL;
	...
}

while the device itself is always present on the returned route, since
ip6_create_rt_rcu() allocates it from res->nh->fib_nh_dev:

net/ipv6/route.c:ip6_create_rt_rcu() {
	struct net_device *dev = res->nh->fib_nh_dev;
	...
	nrt = ip6_dst_alloc(dev_net(dev), dev, flags);
	...
}

Would comparing rt->dst.dev keep the strict-mode contract and make the NULL
guard unnecessary?

Does this also make the two in-tree implementations of the same reverse path
semantics disagree?  nft_fib_ipv6.c derives the device from the fib6
nexthops and never looks at rt6i_idev:

net/ipv6/netfilter/nft_fib_ipv6.c:nft_fib6_info_nh_dev_match() {
	return nh_dev == dev ||
	       l3mdev_master_ifindex_rcu(nh_dev) == dev->ifindex;
}

and the IPv4 half of the same match does the same thing:

net/ipv4/netfilter/ipt_rpfilter.c:rpfilter_lookup_reverse() {
	...
	return fib_info_nh_uses_dev(res.fi, dev) || flags & XT_RPFILTER_LOOSE;
}

So for one topology, nft fib saddr . iif oif would still match while
iptables -m rpfilter in strict mode would not, and the common
-m rpfilter --invert -j DROP rule would then drop the packet.

Two paths where the verdict looks like it changes rather than staying
incidentally the same:

1) The VRF second pass.  vrf_ip6_rcv() re-runs PRE_ROUTING with dev set to
the VRF master:

drivers/net/vrf.c:vrf_ip6_rcv() {
	...
	skb = vrf_rcv_nfhook(NFPROTO_IPV6, NF_INET_PRE_ROUTING, skb, vrf_dev);
	...
}

If the reverse route's nexthop is an enslaved device whose inet6_dev was
torn down, and the fib6_info uses an external nexthop object so the route
survives:

net/ipv6/route.c:fib6_ifdown() {
	...
	if (rt == net->ipv6.fib6_null_entry || rt->nh)
		return 0;
	...
}

then the l3mdev_master_ifindex_rcu() clause that used to accept this is now
skipped entirely.

2) A cached exception route (RTF_CACHE, e.g. a PMTU exception) created while
the nexthop device had no inet6_dev keeps rt6i_idev NULL even after the
device's inet6_dev is recreated, so a packet arriving on that very device is
reported as not matching.

Is the NULL rt6i_idev case meant to be treated as "route exists but device
unknown", or is the intent that the check simply never needed rt6i_idev in
the first place?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906034939.3746540-1-bestswngs%40gmail.com