[PATCH net-next v2 0/2] Fix dangling pointer at f6i->gc_link.

[PATCH net-next v2 0/2] Fix dangling pointer at f6i->gc_link.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

According to a report [1], f6i->gc_link may point to a free block
causing use-after-free. According the stacktraces in the report, it is
very likely that a f6i was added to the GC list after being removed
from the tree of a fib6_table. The reason this can happen is the
current implementation determines if a f6i is on a tree in a wrong
way. It believes a f6i is on a tree if f6i->fib6_table is not NULL.
However, f6i->fib6_table is not reset when f6i is removed from a tree.

The new solution is to check if f6i->fib6_node is not NULL as well.
f6i->fib6_node is set/or reset when the f6i is added/or removed from
from a tree. It can be used to determines if a f6i is on a tree
reliably.

The other change is to determine if a f6i is on a GC list.  The
current implementation relies on RTF_EXPIRES on fib6_flags. It needs
to consider if a f6i is on a tree as well. The new solution is
checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead.

[1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/

---
Major changes from v1:

 - Split fib6_set_expires_locked() and fib6_clean_expires_locked()

 - Use hlist_unhashed() on gc_link instead of checking RTF_EXPIRES to
   determine if a f6i is on a GC list.

 - Add tests on toggling routes between permanent and temporary.

v1: https://lore.kernel.org/all/20231207221627.746324-1-thinker.li@gmail.com/

Kui-Feng Lee (2):
  net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table
    tree.
  selftests: fib_tests: Add tests for toggling between w/ and w/o
    expires.

 include/net/ip6_fib.h                    | 46 ++++++++++++----
 net/ipv6/route.c                         |  6 +--
 tools/testing/selftests/net/fib_tests.sh | 69 +++++++++++++++++++++++-
 3 files changed, 105 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a
f6i (fib6_info) to tb6_gc_hlist.

The current implementation checks if f6i->fib6_table is not NULL to
determines if a f6i is on a tree, however it is not enough. When a f6i is
removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node
is always reset when a f6i is removed from a fib6_table and is set when a
f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
determine if a f6i is on a tree.

The current implementation checks RTF_EXPIRES on f6i->fib6_flags to
determine if a f6i is on a GC list. It also consider if the f6i is on a
tree before making a conclusion. This is indirect and complicated. The new
solution is checking hlist_unhashed(&f6i->gc_link), a clear evidence.

Putting them together, these changes provide more reliable signals to
determines if a f6i should be added/or removed to a GC list.

Fixes: 3dec89b14d37 ("net/ipv6: Remove expired routes with a separated list of routes.")
Reported-by: syzbot+c15aa445274af8674f41@syzkaller.appspotmail.com
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: dsahern@kernel.org
---
 include/net/ip6_fib.h | 46 ++++++++++++++++++++++++++++++++-----------
 net/ipv6/route.c      |  6 +++---
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 1ba9f4ddf2f6..1213722c394f 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -500,21 +500,47 @@ void fib6_gc_cleanup(void);
 
 int fib6_init(void);
 
-/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be
- * NULL.
- */
-static inline void fib6_set_expires_locked(struct fib6_info *f6i,
-					   unsigned long expires)
+static inline void fib6_add_gc_list(struct fib6_info *f6i)
 {
 	struct fib6_table *tb6;
 
 	tb6 = f6i->fib6_table;
-	f6i->expires = expires;
-	if (tb6 && !fib6_has_expires(f6i))
+	if (tb6 &&
+	    rcu_dereference_protected(f6i->fib6_node,
+				      lockdep_is_held(&tb6->tb6_lock)) &&
+	    hlist_unhashed(&f6i->gc_link))
 		hlist_add_head(&f6i->gc_link, &tb6->tb6_gc_hlist);
+}
+
+static inline void fib6_del_gc_list(struct fib6_info *f6i)
+{
+	if (!hlist_unhashed(&f6i->gc_link))
+		hlist_del_init(&f6i->gc_link);
+}
+
+static inline void __fib6_set_expires(struct fib6_info *f6i,
+				      unsigned long expires)
+{
+	f6i->expires = expires;
 	f6i->fib6_flags |= RTF_EXPIRES;
 }
 
+static inline void __fib6_clean_expires(struct fib6_info *f6i)
+{
+	f6i->fib6_flags &= ~RTF_EXPIRES;
+	f6i->expires = 0;
+}
+
+/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be
+ * NULL.
+ */
+static inline void fib6_set_expires_locked(struct fib6_info *f6i,
+					   unsigned long expires)
+{
+	__fib6_set_expires(f6i, expires);
+	fib6_add_gc_list(f6i);
+}
+
 /* fib6_info must be locked by the caller, and fib6_info->fib6_table can be
  * NULL.  If fib6_table is NULL, the fib6_info will no be inserted into the
  * list of GC candidates until it is inserted into a table.
@@ -529,10 +555,8 @@ static inline void fib6_set_expires(struct fib6_info *f6i,
 
 static inline void fib6_clean_expires_locked(struct fib6_info *f6i)
 {
-	if (fib6_has_expires(f6i))
-		hlist_del_init(&f6i->gc_link);
-	f6i->fib6_flags &= ~RTF_EXPIRES;
-	f6i->expires = 0;
+	fib6_del_gc_list(f6i);
+	__fib6_clean_expires(f6i);
 }
 
 static inline void fib6_clean_expires(struct fib6_info *f6i)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b132feae3393..dcaeb88d73aa 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3763,10 +3763,10 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 		rt->dst_nocount = true;
 
 	if (cfg->fc_flags & RTF_EXPIRES)
-		fib6_set_expires_locked(rt, jiffies +
-					clock_t_to_jiffies(cfg->fc_expires));
+		__fib6_set_expires(rt, jiffies +
+				   clock_t_to_jiffies(cfg->fc_expires));
 	else
-		fib6_clean_expires_locked(rt);
+		__fib6_clean_expires(rt);
 
 	if (cfg->fc_protocol == RTPROT_UNSPEC)
 		cfg->fc_protocol = RTPROT_BOOT;
-- 
2.34.1

Re: [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree.

[David Ahern]

On 12/8/23 12:45 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a
> f6i (fib6_info) to tb6_gc_hlist.
> 
> The current implementation checks if f6i->fib6_table is not NULL to
> determines if a f6i is on a tree, however it is not enough. When a f6i is
> removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node
> is always reset when a f6i is removed from a fib6_table and is set when a
> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
> determine if a f6i is on a tree.

Which is an indication that the table is not the right check but neither
is the fib6_node. If expires is set on a route entry, add it to the
gc_list; if expires is reset on a route entry, remove it from the
gc_list. If the value of expires is changed while on the gc_list list,
just update the expires value.

Re: [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree.

[Kui-Feng Lee]

On 12/8/23 14:43, David Ahern wrote:
> On 12/8/23 12:45 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a
>> f6i (fib6_info) to tb6_gc_hlist.
>>
>> The current implementation checks if f6i->fib6_table is not NULL to
>> determines if a f6i is on a tree, however it is not enough. When a f6i is
>> removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node
>> is always reset when a f6i is removed from a fib6_table and is set when a
>> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
>> determine if a f6i is on a tree.
> 
> Which is an indication that the table is not the right check but neither
> is the fib6_node. If expires is set on a route entry, add it to the
> gc_list; if expires is reset on a route entry, remove it from the
> gc_list. If the value of expires is changed while on the gc_list list,
> just update the expires value.
> 

I don't quite follow you.
If an entry is not on a tree, why do we still add the entry to the gc 
list? (This is the reason to check f6i->fib6_node.)

The changes in this patch rely on two indications, 1. if a f6i is on a 
tree, 2. if a f6i is on a gc list (described in the 3rd paragraph of
the comment log.)
An entry is added to a gc list only if it has expires and is not on the 
list yet. An entry is removed from a gc list only if it is on a gc list.
And, just like what you said earlier, it just updates the expires value
while an entry is already on a gc list.

Re: [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree.

[Kui-Feng Lee]

On 12/8/23 15:19, Kui-Feng Lee wrote:
> 
> 
> On 12/8/23 14:43, David Ahern wrote:
>> On 12/8/23 12:45 PM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>>
>>> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before 
>>> inserting a
>>> f6i (fib6_info) to tb6_gc_hlist.
>>>
>>> The current implementation checks if f6i->fib6_table is not NULL to
>>> determines if a f6i is on a tree, however it is not enough. When a 
>>> f6i is
>>> removed from a fib6_table, f6i->fib6_table is not reset. However, 
>>> fib6_node
>>> is always reset when a f6i is removed from a fib6_table and is set 
>>> when a
>>> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
>>> determine if a f6i is on a tree.
>>
>> Which is an indication that the table is not the right check but neither
>> is the fib6_node. If expires is set on a route entry, add it to the
>> gc_list; if expires is reset on a route entry, remove it from the
>> gc_list. If the value of expires is changed while on the gc_list list,
>> just update the expires value.
>>
> 
> I don't quite follow you.
> If an entry is not on a tree, why do we still add the entry to the gc 
> list? (This is the reason to check f6i->fib6_node.)
> 
> The changes in this patch rely on two indications, 1. if a f6i is on a 
> tree, 2. if a f6i is on a gc list (described in the 3rd paragraph of
> the comment log.)
> An entry is added to a gc list only if it has expires and is not on the 
> list yet. An entry is removed from a gc list only if it is on a gc list.
> And, just like what you said earlier, it just updates the expires value
> while an entry is already on a gc list.

Add more context here.

Use rt6_route_rcv() as an example. It looks up or adds a route first.
Then it changes expires of the route at the very end of the function.
There is gap between looking up and the modification. The f6i found here
can be removed from the tree in-between. If the f6i here is added to the
gc list even it has been removed from the tree, fib6_info_release() at 
the end of rt6_route_rcv() might free the f6i while the f6i is still in
the gc list.

This is why it checks if a f6i is on the tree with the protection of
tb6_lock.

[PATCH net-next v2 2/2] selftests: fib_tests: Add tests for toggling between w/ and w/o expires.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Make sure that toggling routes between w/ expires and w/o expires works
properly with GC list.

When a route with expires is replaced by a permanent route, the entry
should be removed from the gc list. When a permanent routes is replaced by
a temporary route, the new entry should be added to the gc list. The new
tests check if these basic operators work properly.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/net/fib_tests.sh | 69 +++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 66d0db7a2614..a8b4628fd7d2 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -806,10 +806,75 @@ fib6_gc_test()
 	    ret=0
 	fi
 
-	set +e
-
 	log_test $ret 0 "ipv6 route garbage collection"
 
+	# Delete permanent routes
+	for i in $(seq 1 5000); do
+	    $IP -6 route del 2001:30::$i \
+		via 2001:10::2 dev dummy_10
+	done
+
+	# Permanent routes
+	for i in $(seq 1 100); do
+	    # Expire route after $EXPIRE seconds
+	    $IP -6 route add 2001:20::$i \
+		via 2001:10::2 dev dummy_10
+	done
+	# Replace with temporary routes
+	for i in $(seq 1 100); do
+	    # Expire route after $EXPIRE seconds
+	    $IP -6 route replace 2001:20::$i \
+		via 2001:10::2 dev dummy_10 expires $EXPIRE
+	done
+	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
+	if [ $N_EXP_SLEEP -ne 100 ]; then
+	    echo "FAIL: expected 100 routes with expires, got $N_EXP_SLEEP"
+	fi
+	sleep $(($EXPIRE * 2 + 1))
+	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
+	if [ $N_EXP_SLEEP -ne 0 ]; then
+	    echo "FAIL: expected 0 routes with expires," \
+		 "got $N_EXP_SLEEP"
+	    ret=1
+	else
+	    ret=0
+	fi
+
+	log_test $ret 0 "ipv6 route garbage collection (replace with expires)"
+
+	PERM_BASE=$($IP -6 route list |grep -v expires|wc -l)
+	# Temporary routes
+	for i in $(seq 1 100); do
+	    # Expire route after $EXPIRE seconds
+	    $IP -6 route add 2001:20::$i \
+		via 2001:10::2 dev dummy_10 expires $EXPIRE
+	done
+	# Replace with permanent routes
+	for i in $(seq 1 100); do
+	    # Expire route after $EXPIRE seconds
+	    $IP -6 route replace 2001:20::$i \
+		via 2001:10::2 dev dummy_10
+	done
+	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
+	if [ $N_EXP_SLEEP -ne 0 ]; then
+	    echo "FAIL: expected 0 routes with expires," \
+		 "got $N_EXP_SLEEP"
+	fi
+	sleep $(($EXPIRE * 2 + 1))
+	N_EXP_PERM=$($IP -6 route list |grep -v expires|wc -l)
+	N_EXP_PERM=$(($N_EXP_PERM - $PERM_BASE))
+	if [ $N_EXP_PERM -ne 100 ]; then
+	    echo "FAIL: expected 100 permanent routes," \
+		 "got $N_EXP_PERM"
+	    ret=1
+	else
+	    ret=0
+	fi
+
+	log_test $ret 0 "ipv6 route garbage collection (replace with permanent)"
+
+	set +e
+
 	cleanup &> /dev/null
 }
 
-- 
2.34.1

Re: [PATCH net-next v2 2/2] selftests: fib_tests: Add tests for toggling between w/ and w/o expires.

[Hangbin Liu]

On Fri, Dec 08, 2023 at 11:45:23AM -0800, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Make sure that toggling routes between w/ expires and w/o expires works
> properly with GC list.
> 
> When a route with expires is replaced by a permanent route, the entry
> should be removed from the gc list. When a permanent routes is replaced by
> a temporary route, the new entry should be added to the gc list. The new
> tests check if these basic operators work properly.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/testing/selftests/net/fib_tests.sh | 69 +++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 66d0db7a2614..a8b4628fd7d2 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -806,10 +806,75 @@ fib6_gc_test()
>  	    ret=0
>  	fi
>  
> -	set +e
> -
>  	log_test $ret 0 "ipv6 route garbage collection"
>  
> +	# Delete permanent routes
> +	for i in $(seq 1 5000); do
> +	    $IP -6 route del 2001:30::$i \
> +		via 2001:10::2 dev dummy_10
> +	done
> +
> +	# Permanent routes
> +	for i in $(seq 1 100); do
> +	    # Expire route after $EXPIRE seconds
> +	    $IP -6 route add 2001:20::$i \
> +		via 2001:10::2 dev dummy_10
> +	done
> +	# Replace with temporary routes
> +	for i in $(seq 1 100); do
> +	    # Expire route after $EXPIRE seconds
> +	    $IP -6 route replace 2001:20::$i \
> +		via 2001:10::2 dev dummy_10 expires $EXPIRE
> +	done
> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
> +	if [ $N_EXP_SLEEP -ne 100 ]; then
> +	    echo "FAIL: expected 100 routes with expires, got $N_EXP_SLEEP"

Hi,

Here the test failed, but ret is not updated.

> +	fi
> +	sleep $(($EXPIRE * 2 + 1))
> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
> +	if [ $N_EXP_SLEEP -ne 0 ]; then
> +	    echo "FAIL: expected 0 routes with expires," \
> +		 "got $N_EXP_SLEEP"
> +	    ret=1

Here the ret is updated.

> +	else
> +	    ret=0
> +	fi
> +
> +	log_test $ret 0 "ipv6 route garbage collection (replace with expires)"
> +
> +	PERM_BASE=$($IP -6 route list |grep -v expires|wc -l)
> +	# Temporary routes
> +	for i in $(seq 1 100); do
> +	    # Expire route after $EXPIRE seconds
> +	    $IP -6 route add 2001:20::$i \
> +		via 2001:10::2 dev dummy_10 expires $EXPIRE
> +	done
> +	# Replace with permanent routes
> +	for i in $(seq 1 100); do
> +	    # Expire route after $EXPIRE seconds
> +	    $IP -6 route replace 2001:20::$i \
> +		via 2001:10::2 dev dummy_10
> +	done
> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
> +	if [ $N_EXP_SLEEP -ne 0 ]; then
> +	    echo "FAIL: expected 0 routes with expires," \
> +		 "got $N_EXP_SLEEP"

Same here.

Thanks
Hangbin
> +	fi
> +	sleep $(($EXPIRE * 2 + 1))
> +	N_EXP_PERM=$($IP -6 route list |grep -v expires|wc -l)
> +	N_EXP_PERM=$(($N_EXP_PERM - $PERM_BASE))
> +	if [ $N_EXP_PERM -ne 100 ]; then
> +	    echo "FAIL: expected 100 permanent routes," \
> +		 "got $N_EXP_PERM"
> +	    ret=1
> +	else
> +	    ret=0
> +	fi
> +
> +	log_test $ret 0 "ipv6 route garbage collection (replace with permanent)"
> +
> +	set +e
> +
>  	cleanup &> /dev/null
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 2/2] selftests: fib_tests: Add tests for toggling between w/ and w/o expires.

[Kui-Feng Lee]

On 12/11/23 18:20, Hangbin Liu wrote:
> On Fri, Dec 08, 2023 at 11:45:23AM -0800, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Make sure that toggling routes between w/ expires and w/o expires works
>> properly with GC list.
>>
>> When a route with expires is replaced by a permanent route, the entry
>> should be removed from the gc list. When a permanent routes is replaced by
>> a temporary route, the new entry should be added to the gc list. The new
>> tests check if these basic operators work properly.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/testing/selftests/net/fib_tests.sh | 69 +++++++++++++++++++++++-
>>   1 file changed, 67 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
>> index 66d0db7a2614..a8b4628fd7d2 100755
>> --- a/tools/testing/selftests/net/fib_tests.sh
>> +++ b/tools/testing/selftests/net/fib_tests.sh
>> @@ -806,10 +806,75 @@ fib6_gc_test()
>>   	    ret=0
>>   	fi
>>   
>> -	set +e
>> -
>>   	log_test $ret 0 "ipv6 route garbage collection"
>>   
>> +	# Delete permanent routes
>> +	for i in $(seq 1 5000); do
>> +	    $IP -6 route del 2001:30::$i \
>> +		via 2001:10::2 dev dummy_10
>> +	done
>> +
>> +	# Permanent routes
>> +	for i in $(seq 1 100); do
>> +	    # Expire route after $EXPIRE seconds
>> +	    $IP -6 route add 2001:20::$i \
>> +		via 2001:10::2 dev dummy_10
>> +	done
>> +	# Replace with temporary routes
>> +	for i in $(seq 1 100); do
>> +	    # Expire route after $EXPIRE seconds
>> +	    $IP -6 route replace 2001:20::$i \
>> +		via 2001:10::2 dev dummy_10 expires $EXPIRE
>> +	done
>> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
>> +	if [ $N_EXP_SLEEP -ne 100 ]; then
>> +	    echo "FAIL: expected 100 routes with expires, got $N_EXP_SLEEP"
> 
> Hi,
> 
> Here the test failed, but ret is not updated.
> 
>> +	fi
>> +	sleep $(($EXPIRE * 2 + 1))
>> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
>> +	if [ $N_EXP_SLEEP -ne 0 ]; then
>> +	    echo "FAIL: expected 0 routes with expires," \
>> +		 "got $N_EXP_SLEEP"
>> +	    ret=1
> 
> Here the ret is updated.
> 
>> +	else
>> +	    ret=0
>> +	fi
>> +
>> +	log_test $ret 0 "ipv6 route garbage collection (replace with expires)"
>> +
>> +	PERM_BASE=$($IP -6 route list |grep -v expires|wc -l)
>> +	# Temporary routes
>> +	for i in $(seq 1 100); do
>> +	    # Expire route after $EXPIRE seconds
>> +	    $IP -6 route add 2001:20::$i \
>> +		via 2001:10::2 dev dummy_10 expires $EXPIRE
>> +	done
>> +	# Replace with permanent routes
>> +	for i in $(seq 1 100); do
>> +	    # Expire route after $EXPIRE seconds
>> +	    $IP -6 route replace 2001:20::$i \
>> +		via 2001:10::2 dev dummy_10
>> +	done
>> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
>> +	if [ $N_EXP_SLEEP -ne 0 ]; then
>> +	    echo "FAIL: expected 0 routes with expires," \
>> +		 "got $N_EXP_SLEEP"
> 
> Same here.
> 
> Thanks
> Hangbin
>> +	fi
>> +	sleep $(($EXPIRE * 2 + 1))
>> +	N_EXP_PERM=$($IP -6 route list |grep -v expires|wc -l)
>> +	N_EXP_PERM=$(($N_EXP_PERM - $PERM_BASE))
>> +	if [ $N_EXP_PERM -ne 100 ]; then
>> +	    echo "FAIL: expected 100 permanent routes," \
>> +		 "got $N_EXP_PERM"
>> +	    ret=1
>> +	else
>> +	    ret=0
>> +	fi
>> +
>> +	log_test $ret 0 "ipv6 route garbage collection (replace with permanent)"
>> +
>> +	set +e
>> +
>>   	cleanup &> /dev/null
>>   }
>>   
>> -- 
>> 2.34.1
>>
Got it! Thanks!

Re: [PATCH net-next v2 2/2] selftests: fib_tests: Add tests for toggling between w/ and w/o expires.

[Hangbin Liu]

On Mon, Dec 11, 2023 at 06:40:43PM -0800, Kui-Feng Lee wrote:
> > > +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
> > > +	if [ $N_EXP_SLEEP -ne 100 ]; then
> > > +	    echo "FAIL: expected 100 routes with expires, got $N_EXP_SLEEP"
> > 
> > Hi,
> > 
> > Here the test failed, but ret is not updated.
> > 
> > > +	fi
> > > +	sleep $(($EXPIRE * 2 + 1))
> > > +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
> > > +	if [ $N_EXP_SLEEP -ne 0 ]; then
> > > +	    echo "FAIL: expected 0 routes with expires," \
> > > +		 "got $N_EXP_SLEEP"
> > > +	    ret=1
> > 
> > Here the ret is updated.

BTW, the current fib6_gc_test() use $ret to store the result. But the latter
check would cover the previous one. e.g.

if [ $N_EXP_SLEEP -ne 0 ]; then
	ret=1
else
	ret=0
fi

do_some_other_tests
if [ $N_EXP_SLEEP -ne 0 ]; then
	ret=1
else
	ret=0
fi

If the previous one failed, but later one pass, the ret would be re-write to 0.
So I think we can use log_test for each checking.

Thanks
Hangbin

Re: [PATCH net-next v2 2/2] selftests: fib_tests: Add tests for toggling between w/ and w/o expires.

[Kui-Feng Lee]

On 12/11/23 21:35, Hangbin Liu wrote:
> On Mon, Dec 11, 2023 at 06:40:43PM -0800, Kui-Feng Lee wrote:
>>>> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
>>>> +	if [ $N_EXP_SLEEP -ne 100 ]; then
>>>> +	    echo "FAIL: expected 100 routes with expires, got $N_EXP_SLEEP"
>>>
>>> Hi,
>>>
>>> Here the test failed, but ret is not updated.
>>>
>>>> +	fi
>>>> +	sleep $(($EXPIRE * 2 + 1))
>>>> +	N_EXP_SLEEP=$($IP -6 route list |grep expires|wc -l)
>>>> +	if [ $N_EXP_SLEEP -ne 0 ]; then
>>>> +	    echo "FAIL: expected 0 routes with expires," \
>>>> +		 "got $N_EXP_SLEEP"
>>>> +	    ret=1
>>>
>>> Here the ret is updated.
> 
> BTW, the current fib6_gc_test() use $ret to store the result. But the latter
> check would cover the previous one. e.g.
> 
> if [ $N_EXP_SLEEP -ne 0 ]; then
> 	ret=1
> else
> 	ret=0
> fi
> 
> do_some_other_tests
> if [ $N_EXP_SLEEP -ne 0 ]; then
> 	ret=1
> else
> 	ret=0
> fi
> 
> If the previous one failed, but later one pass, the ret would be re-write to 0.
> So I think we can use log_test for each checking.
> 
> Thanks
> Hangbin

Some of these tests has dependencies.  So, what I did is to perform the
following commands only if ret = 0.

if [ $N_EXP_SLEEP -ne 0 ]; then
     ret=1
else
     ret=0
fi

if [ $ret -eq 0 ]; then
   .....
   if [ ...]; then
       ret=1
   else
       ret=0
   fi
fi

if [ $ret -eq 0]; then
....
fi

Re: [PATCH net-next v2 0/2] Fix dangling pointer at f6i->gc_link.

[David Ahern]

On 12/8/23 12:45 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> According to a report [1], f6i->gc_link may point to a free block
> causing use-after-free. According the stacktraces in the report, it is
> very likely that a f6i was added to the GC list after being removed
> from the tree of a fib6_table. The reason this can happen is the
> current implementation determines if a f6i is on a tree in a wrong
> way. It believes a f6i is on a tree if f6i->fib6_table is not NULL.
> However, f6i->fib6_table is not reset when f6i is removed from a tree.
> 
> The new solution is to check if f6i->fib6_node is not NULL as well.
> f6i->fib6_node is set/or reset when the f6i is added/or removed from
> from a tree. It can be used to determines if a f6i is on a tree
> reliably.
> 
> The other change is to determine if a f6i is on a GC list.  The
> current implementation relies on RTF_EXPIRES on fib6_flags. It needs
> to consider if a f6i is on a tree as well. The new solution is
> checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead.
> 
> [1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/
> 

Eric: can you release the syzbot report for the backtrace listed in [1].
I would like to make "very likely that a f6i was added to the GC list
after being removed from the tree of a fib6_table" more certain. Thanks,

Re: [PATCH net-next v2 0/2] Fix dangling pointer at f6i->gc_link.

[Eric Dumazet]

On Wed, Dec 13, 2023 at 6:22 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 12/8/23 12:45 PM, thinker.li@gmail.com wrote:
> > From: Kui-Feng Lee <thinker.li@gmail.com>
> >
> > According to a report [1], f6i->gc_link may point to a free block
> > causing use-after-free. According the stacktraces in the report, it is
> > very likely that a f6i was added to the GC list after being removed
> > from the tree of a fib6_table. The reason this can happen is the
> > current implementation determines if a f6i is on a tree in a wrong
> > way. It believes a f6i is on a tree if f6i->fib6_table is not NULL.
> > However, f6i->fib6_table is not reset when f6i is removed from a tree.
> >
> > The new solution is to check if f6i->fib6_node is not NULL as well.
> > f6i->fib6_node is set/or reset when the f6i is added/or removed from
> > from a tree. It can be used to determines if a f6i is on a tree
> > reliably.
> >
> > The other change is to determine if a f6i is on a GC list.  The
> > current implementation relies on RTF_EXPIRES on fib6_flags. It needs
> > to consider if a f6i is on a tree as well. The new solution is
> > checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead.
> >
> > [1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/
> >
>
> Eric: can you release the syzbot report for the backtrace listed in [1].
> I would like to make "very likely that a f6i was added to the GC list
> after being removed from the tree of a fib6_table" more certain. Thanks,

I have one, not sure if the tree is recent enough.

Re: [PATCH net-next v2 0/2] Fix dangling pointer at f6i->gc_link.

[David Ahern]

On 12/13/23 10:32 AM, Eric Dumazet wrote:
> I have one, not sure if the tree is recent enough.

If syzbot is using radvd, can you send that config file?