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

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

[Jiayuan Chen]

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;
+}
+
 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;
+
 	nh_dev = fib6_info_nh_dev(rt);
 	if (nft_fib6_info_nh_dev_match(nh_dev, dev))
 		return true;
 
+	if (!rt->fib6_nsiblings)
+		return false;
+
 	list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {
 		nh_dev = fib6_info_nh_dev(iter);
 
-- 
2.43.0

[PATCH nf 2/2] selftests: netfilter: add nft_fib_nexthop test

[Jiayuan Chen]

Cover nft_fib6_eval() over three route shapes and reproduce the OOB
caused by the blind &rt->fib6_siblings walk:

  1) single external nexthop (nhid)
  2) external nexthop group (nhid -> group)
  3) old-style multipath (nexthop ... nexthop ...)

After the fix:

  ./nft_fib_nexthop.sh
  Nothing to flush
  PASS: single external nexthop (nhid)
  Flushed 1 nexthops
  PASS: nexthop group (nhid group 1/2)
  Flushed 2 nexthops
  PASS: old-style multipath (fib6_siblings)

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 .../testing/selftests/net/netfilter/Makefile  |   1 +
 .../net/netfilter/nft_fib_nexthop.sh          | 104 ++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100755 tools/testing/selftests/net/netfilter/nft_fib_nexthop.sh

diff --git a/tools/testing/selftests/net/netfilter/Makefile b/tools/testing/selftests/net/netfilter/Makefile
index ee2d1a5254f8..d953ee218c0f 100644
--- a/tools/testing/selftests/net/netfilter/Makefile
+++ b/tools/testing/selftests/net/netfilter/Makefile
@@ -26,6 +26,7 @@ TEST_PROGS := \
 	nft_concat_range.sh \
 	nft_conntrack_helper.sh \
 	nft_fib.sh \
+	nft_fib_nexthop.sh \
 	nft_flowtable.sh \
 	nft_interface_stress.sh \
 	nft_meta.sh \
diff --git a/tools/testing/selftests/net/netfilter/nft_fib_nexthop.sh b/tools/testing/selftests/net/netfilter/nft_fib_nexthop.sh
new file mode 100755
index 000000000000..76f934156c8c
--- /dev/null
+++ b/tools/testing/selftests/net/netfilter/nft_fib_nexthop.sh
@@ -0,0 +1,104 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# shellcheck disable=SC2154
+#
+# Exercise nft_fib6_eval()'s sibling/nh enumeration on three route shapes:
+#   1) route via a single external nexthop (nhid)
+#   2) route via an external nexthop group (nhid -> group)
+#   3) route via old-style multipath (nexthop ... nexthop ...)
+#
+# Topology similar to nft_fib.sh, without ns2; two dummy interfaces on
+# nsrouter host the nh devices:
+#
+#   dead:1::99           dead:1::1     dummy0 dead:2::1
+#   ns1 <-----veth----->         nsrouter
+#                                       dummy1 dead:9::1
+
+source lib.sh
+
+ret=0
+
+checktool "nft --version" "run test without nft"
+checktool "ip -V"         "run test without iproute2"
+
+setup_ns nsrouter ns1
+trap cleanup_all_ns EXIT
+
+if ! ip link add veth0 netns "$nsrouter" type veth peer name eth0 netns "$ns1" \
+	> /dev/null 2>&1; then
+	echo "SKIP: No virtual ethernet pair device support in kernel"
+	exit $ksft_skip
+fi
+
+ip -net "$ns1" link set lo up
+ip -net "$ns1" link set eth0 up
+ip -net "$ns1" -6 addr add dead:1::99/64 dev eth0 nodad
+ip -net "$ns1" -6 route add default via dead:1::1
+
+ip -net "$nsrouter" link set lo up
+ip -net "$nsrouter" link set veth0 up
+ip -net "$nsrouter" -6 addr add dead:1::1/64 dev veth0 nodad
+
+if ! ip -net "$nsrouter" link add dummy0 type dummy 2>/dev/null; then
+	echo "SKIP: dummy netdev not available"
+	exit $ksft_skip
+fi
+ip -net "$nsrouter" link set dummy0 up
+ip -net "$nsrouter" -6 addr add dead:2::1/64 dev dummy0 nodad
+
+ip -net "$nsrouter" link add dummy1 type dummy
+ip -net "$nsrouter" link set dummy1 up
+ip -net "$nsrouter" -6 addr add dead:9::1/64 dev dummy1 nodad
+
+ip netns exec "$nsrouter" sysctl -q net.ipv6.conf.all.forwarding=1
+
+load_fib_rule() {
+	ip netns exec "$nsrouter" nft -f /dev/stdin <<EOF
+flush ruleset
+table ip6 t {
+	chain c {
+		type filter hook prerouting priority 0; policy accept;
+		fib daddr . iif oif missing counter
+	}
+}
+EOF
+}
+
+run_scenario() {
+	local what="$1"; shift
+
+	ip -net "$nsrouter" -6 route del dead:dead::/64 2>/dev/null || true
+	ip -net "$nsrouter" -6 nexthop flush 2>/dev/null || true
+
+	"$@" || { echo "SKIP ($what): could not configure route"; return; }
+
+	load_fib_rule || { echo "FAIL ($what): nft load"; ret=1; return; }
+
+	ip netns exec "$ns1" ping -6 -c 1 -W 1 dead:dead::1 \
+		> /dev/null 2>&1 || true
+
+	echo "PASS: $what"
+}
+
+scenario_single_nh() {
+	ip -net "$nsrouter" nexthop add id 1 via dead:2::2 dev dummy0
+	ip -net "$nsrouter" -6 route add dead:dead::/64 nhid 1
+}
+run_scenario "single external nexthop (nhid)" scenario_single_nh
+
+scenario_nh_group() {
+	ip -net "$nsrouter" nexthop add id 1   via dead:2::2 dev dummy0
+	ip -net "$nsrouter" nexthop add id 2   via dead:9::2 dev dummy1
+	ip -net "$nsrouter" nexthop add id 100 group 1/2
+	ip -net "$nsrouter" -6 route   add dead:dead::/64 nhid 100
+}
+run_scenario "nexthop group (nhid group 1/2)" scenario_nh_group
+
+scenario_old_multipath() {
+	ip -net "$nsrouter" -6 route add dead:dead::/64 \
+		nexthop via dead:2::2 dev dummy0 \
+		nexthop via dead:9::2 dev dummy1
+}
+run_scenario "old-style multipath (fib6_siblings)" scenario_old_multipath
+
+exit $ret
-- 
2.43.0

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

[Phil Sutter]

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;

As above, the '!= 0' is not needed, is it?

> +
>  	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")?

> +		return false;
> +
>  	list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {
>  		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

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

[Jiayuan Chen]

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

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

[Phil Sutter]

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

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

[Jiayuan Chen]

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

Re: [PATCH nf 2/2] selftests: netfilter: add nft_fib_nexthop test

[Florian Westphal]

Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> Cover nft_fib6_eval() over three route shapes and reproduce the OOB
> caused by the blind &rt->fib6_siblings walk:
> 
>   1) single external nexthop (nhid)
>   2) external nexthop group (nhid -> group)
>   3) old-style multipath (nexthop ... nexthop ...)

https://sashiko.dev/#/patchset/20260519041431.396218-1-jiayuan.chen%40linux.dev

Specifially: 'Does the test need to verify the datapath actually
evaluated correctly?' and
'It looks like the test also sets up an nftables counter rule but doesn't
verify if the rule actually matched any packets.'

I prefer functionality test over 'bug trap' tests.

Re: [PATCH nf 2/2] selftests: netfilter: add nft_fib_nexthop test

[Jiayuan Chen]

On 5/20/26 2:53 AM, Florian Westphal wrote:
> Specifially: 'Does the test need to verify the datapath actually
> evaluated correctly?' and
> 'It looks like the test also sets up an nftables counter rule but doesn't
> verify if the rule actually matched any packets.'
>
> I prefer functionality test over 'bug trap' tests.


Agreed -- functional verification is the right approach here.