Re: [PATCH nf 1/2] netfilter: nft_fib_ipv6: handle routes via external nexthop

[Jiayuan Chen <jiayuan.chen@linux.dev>]

Hi Phil,

Thanks for your review.

On 5/19/26 6:08 PM, Phil Sutter wrote:
> Hi,
>
> On Tue, May 19, 2026 at 12:14:30PM +0800, Jiayuan Chen wrote:
>> fib6_info has a union:
>>
>>      union {
>>          struct list_head fib6_siblings;
>>          struct list_head nh_list;
>>      };
>>
>> Old-style multipath (ip -6 route add ... nexthop ... nexthop ...) uses
>> fib6_siblings.  External nexthop (ip -6 route add ... nhid N) uses
>> nh_list, linked into &nh->f6i_list.
>>
>> nft_fib6_info_nh_uses_dev() blindly walks &rt->fib6_siblings, causing
>> an OOB read past the struct nexthop slab when rt->nh is set:
>>
>>    ==================================================================
>>    BUG: KASAN: slab-out-of-bounds in nft_fib6_eval+0x1362/0x16c0
>>    Read of size 8 at addr ffff888103a099d0 by task ping/386
>>
>>    CPU: 2 UID: 0 PID: 386 Comm: ping Not tainted 7.1.0-rc3+ #251 PREEMPT
>>    Call Trace:
>>     <IRQ>
>>     dump_stack_lvl+0x76/0xa0
>>     print_report+0xd1/0x5f0
>>     kasan_report+0xe7/0x130
>>     __asan_report_load8_noabort+0x14/0x30
>>     nft_fib6_eval+0x1362/0x16c0
>>     nft_do_chain+0x279/0x18c0
>>     nft_do_chain_ipv6+0x1a8/0x230
>>     nf_hook_slow+0xad/0x200
>>     ipv6_rcv+0x152/0x380
>>     __netif_receive_skb_one_core+0x118/0x1c0
>>    ==================================================================
>>
>> Branch by route shape: when rt->nh is set, walk via
>> nexthop_for_each_fib6_nh() (also covers nh groups, which the original
>> code missed); otherwise walk fib6_siblings, guarded by fib6_nsiblings.
>>
>> Fixes: 1c32b24c234b ("netfilter: nft_fib_ipv6: switch to fib6_lookup")
>> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>> ---
>>   net/ipv6/netfilter/nft_fib_ipv6.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
>> index 8b2dba88ee96..a44919f46de9 100644
>> --- a/net/ipv6/netfilter/nft_fib_ipv6.c
>> +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
>> @@ -160,16 +160,32 @@ static bool nft_fib6_info_nh_dev_match(const struct net_device *nh_dev,
>>   	       l3mdev_master_ifindex_rcu(nh_dev) == dev->ifindex;
>>   }
>>   
>> +static int nft_fib6_nh_match_dev_cb(struct fib6_nh *nh, void *arg)
>> +{
>> +	const struct net_device *dev = arg;
>> +
>> +	return nft_fib6_info_nh_dev_match(nh->fib_nh_dev, dev) ? 1 : 0;
> Why the ternary here? The function returns bool, but the iterator merely
> checks the value for 0 and caller returns the value as bool as well.
>
>> +}
>> +
>>   static bool nft_fib6_info_nh_uses_dev(struct fib6_info *rt,
>>   				      const struct net_device *dev)
>>   {
>>   	const struct net_device *nh_dev;
>>   	struct fib6_info *iter;
>>   
>> +	/* External nexthop: fib6_siblings slot aliases nh_list, walk via nh. */
>> +	if (rt->nh)
>> +		return nexthop_for_each_fib6_nh(rt->nh,
>> +						nft_fib6_nh_match_dev_cb,
>> +						(void *)dev) != 0;


All make sense !


>> +
>>   	nh_dev = fib6_info_nh_dev(rt);
>>   	if (nft_fib6_info_nh_dev_match(nh_dev, dev))
>>   		return true;
>>   
>> +	if (!rt->fib6_nsiblings)
> Should this access using READ_ONCE() as per commit 31d7d67ba127 ("ipv6:
> annotate data-races around rt->fib6_nsiblings")?


You are right, we need READ_ONCE since fib6_add_rt2node will modify 
@fib6_nsiblings .


>> +		return false;
>> +
>>   	list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {


Now I think we should also change list_for_each_entry  into 
list_for_each_entry_rcu for the same reason.

But I'm not sure whether it is appropriate or not since this patch 
target to commit in Fiexes tag.

May be a followup patch is necessary.



>>   		nh_dev = fib6_info_nh_dev(iter);
> I thought about open-coding this to void the need for the callback
> wrapper, but it's not worth it.
>
> Cheers, Phil