Message-ID: <CAH5Ym4iVP0XYrb1=7QhDqhEO54vpSJGFGHaBnuM1qpua1p5-tg@mail.gmail.com>
Date: Wed, 13 Nov 2024 12:43:00 -0800
From: Sam Edwards <cfsworks@...il.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	David Ahern <dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net 2/2] selftests/rtnetlink.sh: add mngtempaddr test

On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@...il.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@...il.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.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
>

Powered by blists - more mailing lists