| Message ID | 20241113125152.752778-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ipv6: fix temporary address not removed correctly | expand |
On Wed, 13 Nov 2024 12:51:52 +0000 Hangbin Liu wrote: > Add a test to check the temporary address could be added/removed > correctly when mngtempaddr is set or removed/unmanaged. Doesn't seem to work for us: # [+300.25] tempaddr not deleted for 2001:db8::1 # [+0.16] tempaddr not deleted for 2003:db8::1 # [+0.07] FAIL: mngtmpaddr add/remove incorrect not ok 1 selftests: net: rtnetlink.sh # exit=1
On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > Add a test to check the temporary address could be added/removed > correctly when mngtempaddr is set or removed/unmanaged. > > Suggested-by: Sam Edwards <cfsworks@gmail.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- Hi Hangbin, Ahh, you beat me to it! I was starting to work on a test case of my own but this looks much better organized. :) I have some comments in cases where our approaches differ: > tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++ > 1 file changed, 89 insertions(+) > > diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh > index bdf6f10d0558..f25a363d55bd 100755 > --- a/tools/testing/selftests/net/rtnetlink.sh > +++ b/tools/testing/selftests/net/rtnetlink.sh > @@ -29,6 +29,7 @@ ALL_TESTS=" > kci_test_bridge_parent_id > kci_test_address_proto > kci_test_enslave_bonding > + kci_test_mngtmpaddr > " > > devdummy="test-dummy0" > @@ -44,6 +45,7 @@ check_err() > if [ $ret -eq 0 ]; then > ret=$1 > fi > + [ -n "$2" ] && echo "$2" > } > > # same but inverted -- used when command must fail for test to pass > @@ -1267,6 +1269,93 @@ kci_test_enslave_bonding() > ip netns del "$testns" > } > > +# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting > +check_tempaddr_exists() > +{ > + local start=${1-"1"} > + addr_list=$(ip -j -n $testns addr show dev ${devdummy}) > + for i in $(seq $start 4); do > + if ! echo ${addr_list} | \ > + jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \ > + grep -q "200${i}"; then > + check_err $? "No mngtmpaddr 200${i}:db8::1" > + return 0 > + fi > + > + if ! echo ${addr_list} | \ > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ > + grep -q "200${i}"; then > + check_err $? "No tempaddr for 200${i}:db8::1" > + return 0 > + fi > + done > + return 1 > +} The variant of this function that I implemented is a lot less "fixed" and gathers all IPv6 prefixes (by /64) into one of 3 sets: 1. mngtmpaddr 2. temporary, not deprecated 3. temporary (whether deprecated or not) It then ensures that set 3 is a subset of set 1, and set 1 is a subset of set 2. (And if it's easy: it should also ensure that no 'temporary' has a *_lft in excess of its parent's.) Doing it this way allows the test case to create, modify, and delete mngtmpaddrs according to the needs of the test, and the check() function only ensures that the rules are being obeyed, it doesn't make assumptions about the expected state of the addresses. > + > +kci_test_mngtmpaddr() > +{ > + local ret=0 > + > + setup_ns testns > + if [ $? -ne 0 ]; then > + end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns" > + return $ksft_skip > + fi > + > + # 1. Create a dummy Ethernet interface > + run_cmd ip -n $testns link add ${devdummy} type dummy > + run_cmd ip -n $testns link set ${devdummy} up > + run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1 Test should also set .temp_prefered_lft and .temp_valid_lft here. I also set .max_desync_factor=0 because this is a dummy interface that doesn't have any latency, which allows the prefer lifetime to be pretty short. (See below.) > + # 2. Create several (3-4) mngtmpaddr addresses on that interface. > + # with temp_*_lft configured to be pretty short (10 and 35 seconds > + # for prefer/valid respectively) > + for i in $(seq 1 4); do > + run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr I don't really like using 200X:db8::1 as the test addresses. 2001:db8::/32 is the IANA designated prefix for examples/documentation (and, by extension, unit tests) so we should really try to remain inside that. Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak) just to minimize the chances of conflicting with something else in the system. Though, with the test happening in its own netns, *that* level of caution may not be necessary. Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there. > + tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \ > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ > + grep 200${i}) > + #3. Confirm that temporary addresses are created immediately. This could simply be a call to the above genericized check() function. > + if [ -z $tempaddr ]; then > + check_err 1 "no tempaddr created for 200${i}:db8::1" > + else > + run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \ > + preferred_lft 10 valid_lft 35 While Linux is (apparently) happy to let userspace modify the tempaddr's remaining lifetime like this, I don't think this is a common or recommended practice. Rather, the test should be letting Linux manage the tempaddr lifetimes and rotate the addresses itself. > + fi > + done Here is a good place to create an address that *isn't* mngtmpaddr, confirm there is no temporary (via call to check() function), then add the `mngtmpaddr` flag after the fact. > + > + #4. Confirm that a preferred temporary address exists for each mngtmpaddr > + # address at all times, polling once per second for at least 5 minutes. > + slowwait 300 check_tempaddr_exists So I previously said "wait 5 minutes" but I later saw in the documentation for the selftest suite that maintainers really don't like it when a test takes more than ~45 seconds to run. We might want to drop this wait to 30 by default and accelerate the timetable on prefer/valid lifetimes to something like 10/25. > + > + #5. Delete each mngtmpaddr address, one at a time (alternating between > + # deleting and merely un-mngtmpaddr-ing), and confirm that the other > + # mngtmpaddr addresses still have preferred temporaries. > + for i in $(seq 1 4); do > + if (( $i % 2 == 1 )); then > + run_cmd ip -n $testns addr del 200${i}:db8::1/64 dev ${devdummy} > + else > + run_cmd ip -n $testns addr change 200${i}:db8::1/64 dev ${devdummy} > + fi > + # the temp addr should be deleted > + if ip -j -n $testns addr show dev ${devdummy} | \ > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ > + grep -q "200${i}"; then > + check_err 1 "tempaddr not deleted for 200${i}:db8::1" > + fi > + # Check other addresses are still exist > + check_tempaddr_exists $((i + 1)) Ditto, the whole bottom half of this loop can be a call to a genericized check function. > + done > + > + if [ $ret -ne 0 ]; then > + end_test "FAIL: mngtmpaddr add/remove incorrect" > + ip netns del "$testns" > + return 1 > + fi > + > + end_test "PASS: mngtmpaddr add/remove correctly" > + ip netns del "$testns" Do we need to make sure the netns gets cleaned up via `trap ... EXIT` so that it doesn't leak if the user interrupts the test? Or does the greater test fixture take care of that for us? Happy Wednesday, Sam > +} > + > kci_test_rtnl() > { > local current_test > -- > 2.46.0 >
On Wed, Nov 13, 2024 at 11:56:12AM -0800, Jakub Kicinski wrote: > On Wed, 13 Nov 2024 12:51:52 +0000 Hangbin Liu wrote: > > Add a test to check the temporary address could be added/removed > > correctly when mngtempaddr is set or removed/unmanaged. > > Doesn't seem to work for us: > > # [+300.25] tempaddr not deleted for 2001:db8::1 > # [+0.16] tempaddr not deleted for 2003:db8::1 > # [+0.07] FAIL: mngtmpaddr add/remove incorrect > not ok 1 selftests: net: rtnetlink.sh # exit=1 Is this tested with patched kernel or unpatched kernel. On my local side I got # ./rtnetlink.sh -t kci_test_mngtmpaddr PASS: mngtmpaddr add/remove correctly Thanks Hangbin
On Thu, 14 Nov 2024 02:00:01 +0000 Hangbin Liu wrote: > > # [+300.25] tempaddr not deleted for 2001:db8::1 > > # [+0.16] tempaddr not deleted for 2003:db8::1 > > # [+0.07] FAIL: mngtmpaddr add/remove incorrect > > not ok 1 selftests: net: rtnetlink.sh # exit=1 > > Is this tested with patched kernel or unpatched kernel. On my local side I got > > # ./rtnetlink.sh -t kci_test_mngtmpaddr > PASS: mngtmpaddr add/remove correctly I believe you that you run the test before sending. But if it doesn't pass in CI we can't merge this. Just a shot in the dark but does it also pass without the -t ?
On Wed, Nov 13, 2024 at 06:43:12PM -0800, Jakub Kicinski wrote: > On Thu, 14 Nov 2024 02:00:01 +0000 Hangbin Liu wrote: > > > # [+300.25] tempaddr not deleted for 2001:db8::1 > > > # [+0.16] tempaddr not deleted for 2003:db8::1 > > > # [+0.07] FAIL: mngtmpaddr add/remove incorrect > > > not ok 1 selftests: net: rtnetlink.sh # exit=1 > > > > Is this tested with patched kernel or unpatched kernel. On my local side I got > > > > # ./rtnetlink.sh -t kci_test_mngtmpaddr > > PASS: mngtmpaddr add/remove correctly > > I believe you that you run the test before sending. > But if it doesn't pass in CI we can't merge this. > Just a shot in the dark but does it also pass without the -t ? Yes. # ./rtnetlink.sh PASS: policy routing ... PASS: enslave interface in a bond PASS: mngtmpaddr add/remove correctly I will modify the test with Sam's suggestions and see if it could pass. Thanks Hangbin
Hi Sam, On Wed, Nov 13, 2024 at 12:43:00PM -0800, Sam Edwards wrote: > > +# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting > > +check_tempaddr_exists() > > +{ > > + local start=${1-"1"} > > + addr_list=$(ip -j -n $testns addr show dev ${devdummy}) > > + for i in $(seq $start 4); do > > + if ! echo ${addr_list} | \ > > + jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \ > > + grep -q "200${i}"; then > > + check_err $? "No mngtmpaddr 200${i}:db8::1" > > + return 0 > > + fi > > + > > + if ! echo ${addr_list} | \ > > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ > > + grep -q "200${i}"; then > > + check_err $? "No tempaddr for 200${i}:db8::1" > > + return 0 > > + fi > > + done > > + return 1 > > +} > > The variant of this function that I implemented is a lot less "fixed" > and gathers all IPv6 prefixes (by /64) into one of 3 sets: > 1. mngtmpaddr > 2. temporary, not deprecated > 3. temporary (whether deprecated or not) > > It then ensures that set 3 is a subset of set 1, and set 1 is a subset > of set 2. (And if it's easy: it should also ensure that no 'temporary' > has a *_lft in excess of its parent's.) I'm not totally get your explanation here. e.g. with preferred_lft 10, valid_lft 30. I got the following result. # ip addr show dummy0 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000 link/ether 2e:f7:df:87:44:64 brd ff:ff:ff:ff:ff:ff inet6 2001::743:ec1e:5c19:404f/64 scope global temporary dynamic valid_lft 25sec preferred_lft 5sec inet6 2001::938f:432:f32d:602f/64 scope global temporary dynamic valid_lft 19sec preferred_lft 0sec inet6 2001::5b65:c0a3:cd8c:edf8/64 scope global temporary deprecated dynamic valid_lft 3sec preferred_lft 0sec inet6 2001::8a7e:6e8d:83f1:9ea0/64 scope global temporary deprecated dynamic valid_lft 0sec preferred_lft 0sec inet6 2001::1/64 scope global mngtmpaddr valid_lft forever preferred_lft forever So there are 1 mngtmpaddr, 2 temporary address (not deprecated). 4 total temporary address. Based on your rule. It should be set 1 is a subset of set 2. Set 2 is a subset of 3. And how do we ensure that no 'temporary' has a *_lft in excess of its parent's. > Doing it this way allows the test case to create, modify, and delete > mngtmpaddrs according to the needs of the test, and the check() > function only ensures that the rules are being obeyed, it doesn't make > assumptions about the expected state of the addresses. I'm not sure if this is totally enough. What if there are 3 mngtmpaddrs and 4 temporary address. But actually 1 mngtmpaddrs doesn't have temporary address. Maybe check() needs to check only 1 prefix each time. > > + > > +kci_test_mngtmpaddr() > > +{ > > + local ret=0 > > + > > + setup_ns testns > > + if [ $? -ne 0 ]; then > > + end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns" > > + return $ksft_skip > > + fi > > + > > + # 1. Create a dummy Ethernet interface > > + run_cmd ip -n $testns link add ${devdummy} type dummy > > + run_cmd ip -n $testns link set ${devdummy} up > > + run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1 > > Test should also set .temp_prefered_lft and .temp_valid_lft here. > > I also set .max_desync_factor=0 because this is a dummy interface that > doesn't have any latency, which allows the prefer lifetime to be > pretty short. (See below.) Thanks, I will fix the test. > > > + # 2. Create several (3-4) mngtmpaddr addresses on that interface. > > + # with temp_*_lft configured to be pretty short (10 and 35 seconds > > + # for prefer/valid respectively) > > + for i in $(seq 1 4); do > > + run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr > > I don't really like using 200X:db8::1 as the test addresses. > 2001:db8::/32 is the IANA designated prefix for examples/documentation > (and, by extension, unit tests) so we should really try to remain > inside that. > > Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak) > just to minimize the chances of conflicting with something else in the > system. Though, with the test happening in its own netns, *that* level > of caution may not be necessary. > > Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there. OK, I will use 2001:db8::/32 for testing. > > > + tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \ > > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ > > + grep 200${i}) > > + #3. Confirm that temporary addresses are created immediately. > > This could simply be a call to the above genericized check() function. > > > + if [ -z $tempaddr ]; then > > + check_err 1 "no tempaddr created for 200${i}:db8::1" > > + else > > + run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \ > > + preferred_lft 10 valid_lft 35 > > While Linux is (apparently) happy to let userspace modify the > tempaddr's remaining lifetime like this, I don't think this is a > common or recommended practice. Rather, the test should be letting > Linux manage the tempaddr lifetimes and rotate the addresses itself. OK > > > + fi > > + done > > Here is a good place to create an address that *isn't* mngtmpaddr, > confirm there is no temporary (via call to check() function), then add > the `mngtmpaddr` flag after the fact. OK, I will > > > + > > + #4. Confirm that a preferred temporary address exists for each mngtmpaddr > > + # address at all times, polling once per second for at least 5 minutes. > > + slowwait 300 check_tempaddr_exists > > So I previously said "wait 5 minutes" but I later saw in the > documentation for the selftest suite that maintainers really don't > like it when a test takes more than ~45 seconds to run. We might want > to drop this wait to 30 by default and accelerate the timetable on > prefer/valid lifetimes to something like 10/25. Yes, 5m is too long for a single test. > > + > > + end_test "PASS: mngtmpaddr add/remove correctly" > > + ip netns del "$testns" > > Do we need to make sure the netns gets cleaned up via `trap ... EXIT` > so that it doesn't leak if the user interrupts the test? Or does the > greater test fixture take care of that for us? No, rtnetlink.sh doesn't have a trap function. I plan to add the trap function separately. Thanks Hangbin
On Thu, 14 Nov 2024 08:19:31 +0000 Hangbin Liu wrote: > > > Is this tested with patched kernel or unpatched kernel. On my local side I got > > > > > > # ./rtnetlink.sh -t kci_test_mngtmpaddr > > > PASS: mngtmpaddr add/remove correctly > > > > I believe you that you run the test before sending. > > But if it doesn't pass in CI we can't merge this. > > Just a shot in the dark but does it also pass without the -t ? > > Yes. > # ./rtnetlink.sh > PASS: policy routing > ... > PASS: enslave interface in a bond > PASS: mngtmpaddr add/remove correctly > > I will modify the test with Sam's suggestions and see if it could pass. The other option is that some other test doesn't clean up after itself, since we reuse VMs. We have a script in NIPA to map things: contest/cithreadmap Looks like we run rtnetlink.sh after pmtu.sh, I wonder if pmtu.sh is to blame: Thread3-VM0 4-pmtu-sh/ 5-rtnetlink-sh/ ....
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh index bdf6f10d0558..f25a363d55bd 100755 --- a/tools/testing/selftests/net/rtnetlink.sh +++ b/tools/testing/selftests/net/rtnetlink.sh @@ -29,6 +29,7 @@ ALL_TESTS=" kci_test_bridge_parent_id kci_test_address_proto kci_test_enslave_bonding + kci_test_mngtmpaddr " devdummy="test-dummy0" @@ -44,6 +45,7 @@ check_err() if [ $ret -eq 0 ]; then ret=$1 fi + [ -n "$2" ] && echo "$2" } # same but inverted -- used when command must fail for test to pass @@ -1267,6 +1269,93 @@ kci_test_enslave_bonding() ip netns del "$testns" } +# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting +check_tempaddr_exists() +{ + local start=${1-"1"} + addr_list=$(ip -j -n $testns addr show dev ${devdummy}) + for i in $(seq $start 4); do + if ! echo ${addr_list} | \ + jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \ + grep -q "200${i}"; then + check_err $? "No mngtmpaddr 200${i}:db8::1" + return 0 + fi + + if ! echo ${addr_list} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep -q "200${i}"; then + check_err $? "No tempaddr for 200${i}:db8::1" + return 0 + fi + done + return 1 +} + +kci_test_mngtmpaddr() +{ + local ret=0 + + setup_ns testns + if [ $? -ne 0 ]; then + end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns" + return $ksft_skip + fi + + # 1. Create a dummy Ethernet interface + run_cmd ip -n $testns link add ${devdummy} type dummy + run_cmd ip -n $testns link set ${devdummy} up + run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1 + # 2. Create several (3-4) mngtmpaddr addresses on that interface. + # with temp_*_lft configured to be pretty short (10 and 35 seconds + # for prefer/valid respectively) + for i in $(seq 1 4); do + run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr + tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep 200${i}) + #3. Confirm that temporary addresses are created immediately. + if [ -z $tempaddr ]; then + check_err 1 "no tempaddr created for 200${i}:db8::1" + else + run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \ + preferred_lft 10 valid_lft 35 + fi + done + + #4. Confirm that a preferred temporary address exists for each mngtmpaddr + # address at all times, polling once per second for at least 5 minutes. + slowwait 300 check_tempaddr_exists + + #5. Delete each mngtmpaddr address, one at a time (alternating between + # deleting and merely un-mngtmpaddr-ing), and confirm that the other + # mngtmpaddr addresses still have preferred temporaries. + for i in $(seq 1 4); do + if (( $i % 2 == 1 )); then + run_cmd ip -n $testns addr del 200${i}:db8::1/64 dev ${devdummy} + else + run_cmd ip -n $testns addr change 200${i}:db8::1/64 dev ${devdummy} + fi + # the temp addr should be deleted + if ip -j -n $testns addr show dev ${devdummy} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep -q "200${i}"; then + check_err 1 "tempaddr not deleted for 200${i}:db8::1" + fi + # Check other addresses are still exist + check_tempaddr_exists $((i + 1)) + done + + if [ $ret -ne 0 ]; then + end_test "FAIL: mngtmpaddr add/remove incorrect" + ip netns del "$testns" + return 1 + fi + + end_test "PASS: mngtmpaddr add/remove correctly" + ip netns del "$testns" +} + kci_test_rtnl() { local current_test
Add a test to check the temporary address could be added/removed correctly when mngtempaddr is set or removed/unmanaged. Suggested-by: Sam Edwards <cfsworks@gmail.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++ 1 file changed, 89 insertions(+)