Message-ID: <ZzW46QZf5rzj4tMp@fedora>
Date: Thu, 14 Nov 2024 08:46:33 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Sam Edwards <cfsworks@...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

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

Powered by blists - more mailing lists