Re: [PATCH net-next 7/9] net/ipv6: support RFC6724 rule 5.5 via subtrees

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260714094030.136317-2-equinox@diac24.net?part=7
---
--- Patch [7]: [PATCH net-next 7/9] net/ipv6: support RFC6724 rule 5.5 via subtrees ---
commit 8c6cdd876548668f6d35fa2db4feb39f16c09a62
Author: David 'equinox' Lamparter <equinox@diac24.net>

net/ipv6: support RFC6724 rule 5.5 via subtrees

This commit implements RFC6724 rule 5.5 for IPv6 source address selection.
It introduces subtrees logic to prefer source addresses in a prefix advertised
by the chosen next-hop.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 0ba46ed518a9a..f5c04928aa142 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
[ ... ]
> @@ -1593,6 +1595,80 @@ static bool ipv6_allow_optimistic_dad(const struct net *net,
>  #endif
>  }
>  
> +/* "source address is preferable if the chosen nexthop advertised it as a PIO"
> + * => consider 'advertised as a PIO' to be 'the routes for the source's subtree
> + *    include one with the same nexthop'
> + *
> + * NB: there is no backtracking in the subtree here, this is intentional -
> + * each prefix seen (and accepted) in PIOs creates essentially a "zone" which
> + * is our search scope.
> + */
> +static int ipv6_saddr_rule5p5(struct ipv6_saddr_score *score,
> +			      struct ipv6_saddr_dst *saddr_dst)
> +{
> +	const struct rt6_info *rt, *cmp_rt;
> +	struct dst_entry *cmp_dst;
> +	struct fib6_info *f6i;
> +	int ret = 0;
> +
> +	rt = container_of(saddr_dst->dst, struct rt6_info, dst);
> +
> +	/* fl6->saddr is ::, cf. check at the top of ipv6_common_get_saddr() */
> +	saddr_dst->fl6->saddr = score->ifa->addr;
> +	cmp_dst = ip6_route_output(saddr_dst->net, saddr_dst->sk,
> +				   saddr_dst->fl6);

[Severity: High]
When ipv6_saddr_rule5p5() passes the caller-provided saddr_dst->fl6 pointer
into ip6_route_output(), does this permanently alter the flow state?

ip6_route_output() calls ip6_route_output_flags_noref(), which
unconditionally mutates fields in the flow struct, such as setting
flowi6_iif to LOOPBACK_IFINDEX. It can also call l3mdev_update_flow()
to alter flowi6_oif.

Since the code only clears the injected saddr via memset() below, could this
cause silent corruption of the network flow state for subsequent lookups,
potentially breaking policy routing or VRF logic?

> +	memset(&saddr_dst->fl6->saddr, 0, sizeof(saddr_dst->fl6->saddr));
> +
> +	if (cmp_dst->error)
> +		goto out_release_dst;
> +
> +	cmp_rt = container_of(cmp_dst, struct rt6_info, dst);
> +
> +	/* this must work if _any_ nexthop matches; the non-subtree best may
> +	 * not be in same order as subtree best
> +	 */
> +	for (f6i = rcu_dereference(cmp_rt->from); f6i;
> +	     f6i = rcu_dereference(f6i->fib6_next)) {
> +		struct fib6_nh *f6n = f6i->fib6_nh;

[Severity: High]
Can this cause a slab-out-of-bounds read if the route uses an external
nexthop?

When an IPv6 route is configured with an external nexthop, fib6_info_alloc()
allocates the fib6_info object without space for the inline fib6_nh flexible
array, and instead sets f6i->nh.

Is it safe to unconditionally dereference f6i->fib6_nh without checking if
f6i->nh is in use?

> +		struct fib6_info *sibling;
> +
> +		/* non-subtree route: says nothing about router advertising this source */
> +		if (f6i->fib6_src.plen == 0)
> +			continue;
> +
> +		if (f6n->nh_common.nhc_dev != saddr_dst->dst->dev ||
> +		    f6n->nh_common.nhc_gw_family != AF_INET6)
> +			continue;
> +
> +		if (ipv6_addr_equal(&f6n->nh_common.nhc_gw.ipv6,
> +				    &rt->rt6i_gateway)) {
> +			ret = 1;
> +			goto out_release_dst;
> +		}
> +
> +		if (!f6i->fib6_nsiblings)
> +			continue;
> +
> +		list_for_each_entry(sibling, &f6i->fib6_siblings, fib6_siblings) {

[Severity: High]
Is this list traversal safe to perform without RCU primitives?

The fib6_siblings list is actively mutated by the routing subsystem using RCU
list primitives like list_add_tail_rcu() and list_del_rcu().

Since ipv6_saddr_rule5p5() is executed locklessly under the rcu_read_lock()
held by its caller, should this be using list_for_each_entry_rcu() to avoid
reading torn or uninitialized pointers on weakly ordered architectures?

> +			f6n = sibling->fib6_nh;

[Severity: High]
Similar to the issue above, can this unconditionally access an unallocated
fib6_nh array if the sibling route uses an external nexthop?

> +
> +			if (f6n->nh_common.nhc_dev != saddr_dst->dst->dev ||
> +			    f6n->nh_common.nhc_gw_family != AF_INET6)
> +				continue;
> +
> +			if (ipv6_addr_equal(&f6n->nh_common.nhc_gw.ipv6,
> +					    &rt->rt6i_gateway)) {
> +				ret = 1;
> +				goto out_release_dst;
> +			}
> +		}
> +	}
> +
> +out_release_dst:
> +	dst_release(cmp_dst);
> +	return ret;
> +}
[ ... ]