Message-ID: <CAH5Ym4jGZou27-bwGqxHAf2AHWXpT0=wOa0XNNuqtG9OOhi8EQ@mail.gmail.com>
Date: Fri, 15 Nov 2024 12:59:27 -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 Thu, Nov 14, 2024 at 12:46 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> 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.

Hi Hangbin,

My apologies, I should have shared my version of the check function
before. Here it is:

```bash
# Called to validate the addresses on $IFNAME:
#
# 1. Every `temporary` address must have a matching `mngtmpaddr`
# 2. Every `mngtmpaddr` address must have some un`deprecated` `temporary`
#
# Fails the whole test script if a problem is detected, else returns silently.
validate()
{
    mng_prefixes=()
    undep_prefixes=()
    temp_addrs=()

    while read -r line; do
        line_array=($line)
        address="${line_array[1]}"
        prefix="$(echo "$address" | cut -d: -f1-4)::/64"
        if echo "$line" | grep -q mngtmpaddr; then
            mng_prefixes+=("$prefix")
        elif echo "$line" | grep -q temporary; then
            temp_addrs+=("$address")
            if echo "$line" | grep -qv deprecated; then
                undep_prefixes+=("$prefix")
            fi
        fi
    done < <(ip -6 addr show dev $IFNAME | grep '^\s*inet6')

    # 1. All temporary addresses (temp and dep) must have a matching mngtmpaddr
    for address in ${temp_addrs[@]}; do
        prefix="$(echo "$address" | cut -d: -f1-4)::/64"
        if [[ ! " ${mng_prefixes[*]} " =~ " $prefix " ]]; then
            echo "FAIL: Temporary $address with no matching mngtmpaddr!";
            exit 1
        fi
    done

    # 2. All mngtmpaddr addresses must have a temporary address (not dep)
    for prefix in ${mng_prefixes[@]}; do
        if [[ ! " ${undep_prefixes[*]} " =~ " $prefix " ]]; then
            echo "FAIL: No undeprecated temporary in $prefix!";
            exit 1
        fi
    done
}
```

Of course this is using awful text parsing and not JSON output. But
the idea is that it groups addresses by their /64 prefix, to confirm
that every /64 containing a mngtmpaddrs address also contains an
undeprecated temporary, and that every /64 containing a temporary
(deprecated or not) contains a mngtmpaddrs.

This can be extended for the lifetime checking: when we build the set
of mngtmpaddrs /64s, we also note the valid/preferred_life_time values
for each mngtmpaddr. Then later when we confirm rule 1 (all temporary
addresses must have a matching mngtmpaddr) we also confirm that each
temporary does not outlive the mngtmpaddr in the same /64.

Happy Friday,
Sam

>
> > > +
> > > +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