Re: [PATCH net 2/2] selftests/rtnetlink.sh: add mngtempaddr test

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