netdev - Re: [PATCH net 3/3] selftests: net: fib_nexthops: add test for group refcount imbalance bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5694713b-0d18-27a9-417c-9c500ce19c14@nvidia.com>
Date:   Sun, 21 Nov 2021 19:59:16 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Ido Schimmel <idosch@...sch.org>,
        Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        dsahern@...il.com
Subject: Re: [PATCH net 3/3] selftests: net: fib_nexthops: add test for group
 refcount imbalance bug

On 21/11/2021 19:53, Ido Schimmel wrote:
> On Sun, Nov 21, 2021 at 05:24:53PM +0200, Nikolay Aleksandrov wrote:
>> From: Nikolay Aleksandrov <nikolay@...dia.com>
>>
>> The new selftest runs a sequence which causes circular refcount
>> dependency between deleted objects which cannot be released and results
>> in a netdevice refcount imbalance.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@...dia.com>
>> ---
>>  tools/testing/selftests/net/fib_nexthops.sh | 56 +++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
>> index b5a69ad191b0..48d88a36ae27 100755
>> --- a/tools/testing/selftests/net/fib_nexthops.sh
>> +++ b/tools/testing/selftests/net/fib_nexthops.sh
>> @@ -629,6 +629,59 @@ ipv6_fcnal()
>>  	log_test $? 0 "Nexthops removed on admin down"
>>  }
>>  
>> +ipv6_grp_refs()
>> +{
>> +	run_cmd "$IP link set dev veth1 up"
>> +	run_cmd "$IP link add veth1.10 link veth1 up type vlan id 10"
>> +	run_cmd "$IP link add veth1.20 link veth1 up type vlan id 20"
>> +	run_cmd "$IP -6 addr add 2001:db8:91::1/64 dev veth1.10"
>> +	run_cmd "$IP -6 addr add 2001:db8:92::1/64 dev veth1.20"
>> +	run_cmd "$IP -6 neigh add 2001:db8:91::2 lladdr 00:11:22:33:44:55 dev veth1.10"
>> +	run_cmd "$IP -6 neigh add 2001:db8:92::2 lladdr 00:11:22:33:44:55 dev veth1.20"
>> +	run_cmd "$IP nexthop add id 100 via 2001:db8:91::2 dev veth1.10"
>> +	run_cmd "$IP nexthop add id 101 via 2001:db8:92::2 dev veth1.20"
>> +	run_cmd "$IP nexthop add id 102 group 100"
>> +	run_cmd "$IP route add 2001:db8:101::1/128 nhid 102"
>> +
>> +	# create per-cpu dsts through nh 100
>> +	run_cmd "ip netns exec me mausezahn -6 veth1.10 -B 2001:db8:101::1 -A 2001:db8:91::1 -c 5 -t tcp "dp=1-1023, flags=syn" >/dev/null 2>&1"
> 
> I see that other test cases in this file that are using mausezahn check
> that it exists. See ipv4_torture() for example
> 

Indeed, I'll adjust the test

>> +
>> +	# remove nh 100 from the group to delete the route potentially leaving
>> +	# a stale per-cpu dst
> 
> Not sure I understand the comment. Maybe:
> 
> "Remove nh 100 from the group. If the bug described in the previous
> commit is not fixed, the nexthop continues to cache a per-CPU dst entry
> that holds a reference on the IPv6 route."
> 
> ?
> 

Yes, that is the stale per-cpu dst.

>> +	run_cmd "$IP nexthop replace id 102 group 101"
>> +	run_cmd "$IP route del 2001:db8:101::1/128"
>> +
>> +	# add both nexthops to the group so a reference is taken on them
>> +	run_cmd "$IP nexthop replace id 102 group 100/101"
>> +
>> +	# if the bug exists at this point we have an unlinked IPv6 route
> 
> I would mention that by "the bug" you are referring to the bug described
> in previous commit
> 

since there is no commit id yet, I can give a brief description only
I'll may refer to it by subject though

>> +	# (but not freed due to stale dst) with a reference over the group
>> +	# so we delete the group which will again only unlink it due to the
>> +	# route reference
>> +	run_cmd "$IP nexthop del id 102"
>> +
>> +	# delete the nexthop with stale dst, since we have an unlinked
>> +	# group with a ref to it and an unlinked IPv6 route with ref to the
>> +	# group, the nh will only be unlinked and not freed so the stale dst
>> +	# remains forever and we get a net device refcount imbalance
>> +	run_cmd "$IP nexthop del id 100"
>> +
>> +	# if the bug exists this command will hang because the net device
>> +	# cannot be removed
>> +	timeout -s KILL 5 ip netns exec me ip link del veth1.10 >/dev/null 2>&1
>> +
>> +	# we can't cleanup if the command is hung trying to delete the netdev
>> +	if [ $? -eq 137 ]; then
>> +		return 1
>> +	fi
>> +
>> +	# cleanup
>> +	run_cmd "$IP link del veth1.20"
>> +	run_cmd "$IP nexthop flush"
>> +
>> +	return 0
>> +}
>> +
>>  ipv6_grp_fcnal()
>>  {
>>  	local rc
>> @@ -734,6 +787,9 @@ ipv6_grp_fcnal()
>>  
>>  	run_cmd "$IP nexthop add id 108 group 31/24"
>>  	log_test $? 2 "Nexthop group can not have a blackhole and another nexthop"
>> +
>> +	ipv6_grp_refs
>> +	log_test $? 0 "Nexthop group replace refcounts"
>>  }
>>  
>>  ipv6_res_grp_fcnal()
>> -- 
>> 2.31.1
>>