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

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

On 5/19/26 10:15 PM, Phil Sutter wrote:
> On Tue, May 19, 2026 at 06:50:55PM +0800, Jiayuan Chen wrote:
> [...]
>> 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.
> Seems legit! I missed that one because most examples in net/ipv6/route.c
> use a non-RCU variant, I guess because exclusive access is ensured via
> other means.
>
>> 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.
> I'd submit the _rcu fix in an initial patch of the series, so the one
> introducing the fib6_nsiblings check extends correct code (using _rcu)
> with a correct pattern (READ_ONCE).
>
> Thanks, Phil



Got it - will respin as v2.
Sending after a short cool-down.

Respect