[PATCH net 0/2] ipv6: fix temporary address not removed correctly

[PATCH net 0/2] ipv6: fix temporary address not removed correctly

[Hangbin Liu]

Currently the temporary address is not removed when mngtmpaddr is deleted
or becomes unmanaged. The patch set fixed this issue and add a related
test.

Hangbin Liu (2):
  net/ipv6: delete temporary address if mngtmpaddr is removed or
    un-mngtmpaddr
  selftests/rtnetlink.sh: add mngtempaddr test

 net/ipv6/addrconf.c                      |  5 ++
 tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

-- 
2.46.0

[PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[Hangbin Liu]

RFC8981 section 3.4 says that existing temporary addresses must have their
lifetimes adjusted so that no temporary addresses should ever remain "valid"
or "preferred" longer than the incoming SLAAC Prefix Information. This would
strongly imply in Linux's case that if the "mngtmpaddr" address is deleted or
un-flagged as such, its corresponding temporary addresses must be cleared out
right away.

But now the temporary address is renewed even after ‘mngtmpaddr’ is removed
or becomes unmanaged. Fix this by deleting the temporary address with this
situation.

Fixes: 778964f2fdf0 ("ipv6/addrconf: fix timing bug in tempaddr regen")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/addrconf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 94dceac52884..6852dbce5a7d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4644,6 +4644,10 @@ static void addrconf_verify_rtnl(struct net *net)
 			    !ifp->regen_count && ifp->ifpub) {
 				/* This is a non-regenerated temporary addr. */
 
+				if ((!ifp->valid_lft && !ifp->prefered_lft) ||
+				    ifp->ifpub->state == INET6_IFADDR_STATE_DEAD)
+					goto delete_ifp;
+
 				unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
 
 				if (age + regen_advance >= ifp->prefered_lft) {
@@ -4671,6 +4675,7 @@ static void addrconf_verify_rtnl(struct net *net)
 
 			if (ifp->valid_lft != INFINITY_LIFE_TIME &&
 			    age >= ifp->valid_lft) {
+delete_ifp:
 				spin_unlock(&ifp->lock);
 				in6_ifa_hold(ifp);
 				rcu_read_unlock_bh();
-- 
2.46.0

Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[Sam Edwards]

On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> RFC8981 section 3.4 says that existing temporary addresses must have their
> lifetimes adjusted so that no temporary addresses should ever remain "valid"
> or "preferred" longer than the incoming SLAAC Prefix Information. This would
> strongly imply in Linux's case that if the "mngtmpaddr" address is deleted or
> un-flagged as such, its corresponding temporary addresses must be cleared out
> right away.
>
> But now the temporary address is renewed even after ‘mngtmpaddr’ is removed
> or becomes unmanaged. Fix this by deleting the temporary address with this
> situation.

Hi Hangbin,

Is it actually a new temporary, or is it just failing to purge the old
temporaries? While trying to understand this bug on my own, I learned
about a commit [1] that tried to address the former problem, and it's
possible that the approach in that commit needs to be tightened up
instead.

[1]: 69172f0bcb6a09 ("ipv6 addrconf: fix bug where deleting a
mngtmpaddr can create a new temporary address")

>
> Fixes: 778964f2fdf0 ("ipv6/addrconf: fix timing bug in tempaddr regen")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/addrconf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 94dceac52884..6852dbce5a7d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4644,6 +4644,10 @@ static void addrconf_verify_rtnl(struct net *net)
>                             !ifp->regen_count && ifp->ifpub) {
>                                 /* This is a non-regenerated temporary addr. */
>
> +                               if ((!ifp->valid_lft && !ifp->prefered_lft) ||
> +                                   ifp->ifpub->state == INET6_IFADDR_STATE_DEAD)
> +                                       goto delete_ifp;
> +

My understanding is that the kernel already calls
`manage_tempaddrs(dev, ifp, 0, 0, false, now);` when some `ifp` needs
its temporaries flushed out. That zeroes out the lifetimes of every
temporary, which allows the "destructive" if/elseif/elseif/... block
below to delete it. I believe fixing this bug properly will involve
first understanding why *that* mechanism isn't working as designed.

In any case, this 'if' block is for temporary addresses /which haven't
yet begun their regeneration process/, so this won't work to purge out
addresses that have already started trying to create their
replacement. That'll be a rare and frustrating race for someone in the
future to debug indeed. As well, I broke this 'if' out from the below
if/elseif/elseif/... to establish a clear separation between the
"constructive" parts of an address's lifecycle (currently, only
temporary addresses needing to regenerate) and the "destructive" parts
(the address gradually becoming less prominent as its lifetime runs
out), so destructive/delete logic doesn't belong up here anyway.

What are your thoughts?

Happy Wednesday,
Sam

>                                 unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
>
>                                 if (age + regen_advance >= ifp->prefered_lft) {
> @@ -4671,6 +4675,7 @@ static void addrconf_verify_rtnl(struct net *net)
>
>                         if (ifp->valid_lft != INFINITY_LIFE_TIME &&
>                             age >= ifp->valid_lft) {
> +delete_ifp:
>                                 spin_unlock(&ifp->lock);
>                                 in6_ifa_hold(ifp);
>                                 rcu_read_unlock_bh();
> --
> 2.46.0
>

Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[Hangbin Liu]

On Wed, Nov 13, 2024 at 01:03:13PM -0800, Sam Edwards wrote:
> On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> >
> > RFC8981 section 3.4 says that existing temporary addresses must have their
> > lifetimes adjusted so that no temporary addresses should ever remain "valid"
> > or "preferred" longer than the incoming SLAAC Prefix Information. This would
> > strongly imply in Linux's case that if the "mngtmpaddr" address is deleted or
> > un-flagged as such, its corresponding temporary addresses must be cleared out
> > right away.
> >
> > But now the temporary address is renewed even after ‘mngtmpaddr’ is removed
> > or becomes unmanaged. Fix this by deleting the temporary address with this
> > situation.
> 
> Hi Hangbin,
> 
> Is it actually a new temporary, or is it just failing to purge the old
> temporaries? While trying to understand this bug on my own, I learned

1. If delete the mngtmpaddr with the mngtmpaddr flag. e.g.
`ip addr del 2001::1/64 mngtmpaddr dev dummy0`. The following code in
inet6_addr_del()

    if (!(ifp->flags & IFA_F_TEMPORARY) &&
        (ifa_flags & IFA_F_MANAGETEMPADDR))
            manage_tempaddrs(idev, ifp, 0, 0, false,
                             jiffies);

will be called and the valid_lft/prefered_lft of tempaddres will be set to 0.

2. If we using cmd `ip addr change 2001::1/64 dev dummy0`, the following code in
inet6_addr_modify():

    if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
            if (was_managetempaddr &&
                !(ifp->flags & IFA_F_MANAGETEMPADDR)) {
                    cfg->valid_lft = 0;
                    cfg->preferred_lft = 0;
            }
            manage_tempaddrs(ifp->idev, ifp, cfg->valid_lft,
                             cfg->preferred_lft, !was_managetempaddr,
                             jiffies);
    }
will be called and valid_lft/prefered_lft of tempaddres will be set to 0.

But these 2 setting actually not work as in addrconf_verify_rtnl():

    if ((ifp->flags&IFA_F_TEMPORARY) &&
        !(ifp->flags&IFA_F_TENTATIVE) &&
        ifp->prefered_lft != INFINITY_LIFE_TIME &&
        !ifp->regen_count && ifp->ifpub) {
            /* This is a non-regenerated temporary addr. */

            unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);

            if (age + regen_advance >= ifp->prefered_lft) {

                 ^^ this will always true since prefered_lft is 0

So later we will call ipv6_create_tempaddr(ifpub, true) to create a new
tempaddr.

3. If we delete the mngtmpaddr without the mngtmpaddr flag. e.g.
`ip addr del 2001::1/64 dev dummy0` The following code in inet6_addr_del()

    if (!(ifp->flags & IFA_F_TEMPORARY) &&
        (ifa_flags & IFA_F_MANAGETEMPADDR))
            manage_tempaddrs(idev, ifp, 0, 0, false,
                             jiffies);

Will *not* be called as ifa_flags doesn't have IFA_F_MANAGETEMPADDR.
So in addrconf_verify_rtnl(), the (age + regen_advance >= ifp->prefered_lft)
checking will be false, no new tempaddr will be created. But the later
(ifp->valid_lft != INFINITY_LIFE_TIME && age >= ifp->valid_lft) checking is
also false unless the valid_lft is just timeout. So the tempaddr will be keep
until it's life timeout.

> about a commit [1] that tried to address the former problem, and it's
> possible that the approach in that commit needs to be tightened up
> instead.
> 
> [1]: 69172f0bcb6a09 ("ipv6 addrconf: fix bug where deleting a
> mngtmpaddr can create a new temporary address")

The situation in this patch is that the user removed the temporary address
first. The temporary address is not in the addr list anymore and
addrconf_verify_rtnl() won't create a new one. But later when delete the
mngtmpaddr, the manage_tempaddrs() is called again (because the mngtmpaddr
flag in delete cmd) and a new tempaddr is created.

> 
> >
> > Fixes: 778964f2fdf0 ("ipv6/addrconf: fix timing bug in tempaddr regen")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  net/ipv6/addrconf.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 94dceac52884..6852dbce5a7d 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -4644,6 +4644,10 @@ static void addrconf_verify_rtnl(struct net *net)
> >                             !ifp->regen_count && ifp->ifpub) {
> >                                 /* This is a non-regenerated temporary addr. */
> >
> > +                               if ((!ifp->valid_lft && !ifp->prefered_lft) ||
> > +                                   ifp->ifpub->state == INET6_IFADDR_STATE_DEAD)
> > +                                       goto delete_ifp;
> > +
> 
> My understanding is that the kernel already calls
> `manage_tempaddrs(dev, ifp, 0, 0, false, now);` when some `ifp` needs
> its temporaries flushed out. That zeroes out the lifetimes of every
> temporary, which allows the "destructive" if/elseif/elseif/... block
> below to delete it. I believe fixing this bug properly will involve
> first understanding why *that* mechanism isn't working as designed.

Please see the up comment.

> 
> In any case, this 'if' block is for temporary addresses /which haven't
> yet begun their regeneration process/, so this won't work to purge out
> addresses that have already started trying to create their
> replacement. That'll be a rare and frustrating race for someone in the
> future to debug indeed. As well, I broke this 'if' out from the below
> if/elseif/elseif/... to establish a clear separation between the
> "constructive" parts of an address's lifecycle (currently, only
> temporary addresses needing to regenerate) and the "destructive" parts
> (the address gradually becoming less prominent as its lifetime runs
> out), so destructive/delete logic doesn't belong up here anyway.

Currently my fix is checking the tempaddr valid_lft and ifp->ifpub->state.
Maybe we can delete the tempaddr direcly in ipv6_del_addr() and
inet6_addr_modify()?

Thanks
Hangbin
> 
> What are your thoughts?
> 
> Happy Wednesday,
> Sam
> 
> >                                 unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
> >
> >                                 if (age + regen_advance >= ifp->prefered_lft) {
> > @@ -4671,6 +4675,7 @@ static void addrconf_verify_rtnl(struct net *net)
> >
> >                         if (ifp->valid_lft != INFINITY_LIFE_TIME &&
> >                             age >= ifp->valid_lft) {
> > +delete_ifp:
> >                                 spin_unlock(&ifp->lock);
> >                                 in6_ifa_hold(ifp);
> >                                 rcu_read_unlock_bh();
> > --
> > 2.46.0
> >

Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[Sam Edwards]

On Wed, Nov 13, 2024 at 11:38 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Wed, Nov 13, 2024 at 01:03:13PM -0800, Sam Edwards wrote:
> > On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > RFC8981 section 3.4 says that existing temporary addresses must have their
> > > lifetimes adjusted so that no temporary addresses should ever remain "valid"
> > > or "preferred" longer than the incoming SLAAC Prefix Information. This would
> > > strongly imply in Linux's case that if the "mngtmpaddr" address is deleted or
> > > un-flagged as such, its corresponding temporary addresses must be cleared out
> > > right away.
> > >
> > > But now the temporary address is renewed even after ‘mngtmpaddr’ is removed
> > > or becomes unmanaged. Fix this by deleting the temporary address with this
> > > situation.
> >
> > Hi Hangbin,
> >
> > Is it actually a new temporary, or is it just failing to purge the old
> > temporaries? While trying to understand this bug on my own, I learned
>
> 1. If delete the mngtmpaddr with the mngtmpaddr flag. e.g.
> `ip addr del 2001::1/64 mngtmpaddr dev dummy0`. The following code in
> inet6_addr_del()
>
>     if (!(ifp->flags & IFA_F_TEMPORARY) &&
>         (ifa_flags & IFA_F_MANAGETEMPADDR))
>             manage_tempaddrs(idev, ifp, 0, 0, false,
>                              jiffies);
>
> will be called and the valid_lft/prefered_lft of tempaddres will be set to 0.
>
> 2. If we using cmd `ip addr change 2001::1/64 dev dummy0`, the following code in
> inet6_addr_modify():
>
>     if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
>             if (was_managetempaddr &&
>                 !(ifp->flags & IFA_F_MANAGETEMPADDR)) {
>                     cfg->valid_lft = 0;
>                     cfg->preferred_lft = 0;
>             }
>             manage_tempaddrs(ifp->idev, ifp, cfg->valid_lft,
>                              cfg->preferred_lft, !was_managetempaddr,
>                              jiffies);
>     }
> will be called and valid_lft/prefered_lft of tempaddres will be set to 0.
>
> But these 2 setting actually not work as in addrconf_verify_rtnl():
>
>     if ((ifp->flags&IFA_F_TEMPORARY) &&
>         !(ifp->flags&IFA_F_TENTATIVE) &&
>         ifp->prefered_lft != INFINITY_LIFE_TIME &&
>         !ifp->regen_count && ifp->ifpub) {
>             /* This is a non-regenerated temporary addr. */
>
>             unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
>
>             if (age + regen_advance >= ifp->prefered_lft) {
>
>                  ^^ this will always true since prefered_lft is 0
>
> So later we will call ipv6_create_tempaddr(ifpub, true) to create a new
> tempaddr.
>
> 3. If we delete the mngtmpaddr without the mngtmpaddr flag. e.g.
> `ip addr del 2001::1/64 dev dummy0` The following code in inet6_addr_del()
>
>     if (!(ifp->flags & IFA_F_TEMPORARY) &&
>         (ifa_flags & IFA_F_MANAGETEMPADDR))
>             manage_tempaddrs(idev, ifp, 0, 0, false,
>                              jiffies);
>
> Will *not* be called as ifa_flags doesn't have IFA_F_MANAGETEMPADDR.
> So in addrconf_verify_rtnl(), the (age + regen_advance >= ifp->prefered_lft)
> checking will be false, no new tempaddr will be created. But the later
> (ifp->valid_lft != INFINITY_LIFE_TIME && age >= ifp->valid_lft) checking is
> also false unless the valid_lft is just timeout. So the tempaddr will be keep
> until it's life timeout.
>
> > about a commit [1] that tried to address the former problem, and it's
> > possible that the approach in that commit needs to be tightened up
> > instead.
> >
> > [1]: 69172f0bcb6a09 ("ipv6 addrconf: fix bug where deleting a
> > mngtmpaddr can create a new temporary address")
>
> The situation in this patch is that the user removed the temporary address
> first. The temporary address is not in the addr list anymore and
> addrconf_verify_rtnl() won't create a new one. But later when delete the
> mngtmpaddr, the manage_tempaddrs() is called again (because the mngtmpaddr
> flag in delete cmd) and a new tempaddr is created.
>
> >
> > >
> > > Fixes: 778964f2fdf0 ("ipv6/addrconf: fix timing bug in tempaddr regen")
> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > ---
> > >  net/ipv6/addrconf.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > > index 94dceac52884..6852dbce5a7d 100644
> > > --- a/net/ipv6/addrconf.c
> > > +++ b/net/ipv6/addrconf.c
> > > @@ -4644,6 +4644,10 @@ static void addrconf_verify_rtnl(struct net *net)
> > >                             !ifp->regen_count && ifp->ifpub) {
> > >                                 /* This is a non-regenerated temporary addr. */
> > >
> > > +                               if ((!ifp->valid_lft && !ifp->prefered_lft) ||
> > > +                                   ifp->ifpub->state == INET6_IFADDR_STATE_DEAD)
> > > +                                       goto delete_ifp;
> > > +
> >
> > My understanding is that the kernel already calls
> > `manage_tempaddrs(dev, ifp, 0, 0, false, now);` when some `ifp` needs
> > its temporaries flushed out. That zeroes out the lifetimes of every
> > temporary, which allows the "destructive" if/elseif/elseif/... block
> > below to delete it. I believe fixing this bug properly will involve
> > first understanding why *that* mechanism isn't working as designed.
>
> Please see the up comment.
>
> >
> > In any case, this 'if' block is for temporary addresses /which haven't
> > yet begun their regeneration process/, so this won't work to purge out
> > addresses that have already started trying to create their
> > replacement. That'll be a rare and frustrating race for someone in the
> > future to debug indeed. As well, I broke this 'if' out from the below
> > if/elseif/elseif/... to establish a clear separation between the
> > "constructive" parts of an address's lifecycle (currently, only
> > temporary addresses needing to regenerate) and the "destructive" parts
> > (the address gradually becoming less prominent as its lifetime runs
> > out), so destructive/delete logic doesn't belong up here anyway.
>
> Currently my fix is checking the tempaddr valid_lft and ifp->ifpub->state.
> Maybe we can delete the tempaddr direcly in ipv6_del_addr() and
> inet6_addr_modify()?

Hi Hangbin,

It took me a while to grasp but the problem seems to be a confusion
about what it means to set a temporary's lifetimes to 0/0:
1) "The mngtmpaddrs has gone away; this temporary is slated for
deletion by addrconf_verify_rtnl()"
2) "This temporary address itself shall no longer be used, regenerate
it immediately."

The existing behavior makes sense for the #2 case, but not for the #1
case. It seems sensible to me to keep the #2 behavior as-is, because
userspace might be setting a 0/0 lifetime to forcibly rotate the
temporary.

So it sounds like (at least) one of three fixes is in order:
a) Make ipv6_create_tempaddr() verify that the `ifp` is (still)
alive+mngtmpaddrs, returning with an error code if not.
b) Look at the 3 callsites for ipv6_create_tempaddr() and add the
above verifications before calling.
c) Add a function that calls ipv6_del_addr(temp) for every temporary
with a specified ifpub, and use it instead of manage_tempaddrs(..., 0,
0, false, ...) when deleting/unflagging a mngtmpaddrs.

Personally I like option C the best. What are your thoughts?

Cheers,
Sam

>
> Thanks
> Hangbin
> >
> > What are your thoughts?
> >
> > Happy Wednesday,
> > Sam
> >
> > >                                 unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
> > >
> > >                                 if (age + regen_advance >= ifp->prefered_lft) {
> > > @@ -4671,6 +4675,7 @@ static void addrconf_verify_rtnl(struct net *net)
> > >
> > >                         if (ifp->valid_lft != INFINITY_LIFE_TIME &&
> > >                             age >= ifp->valid_lft) {
> > > +delete_ifp:
> > >                                 spin_unlock(&ifp->lock);
> > >                                 in6_ifa_hold(ifp);
> > >                                 rcu_read_unlock_bh();
> > > --
> > > 2.46.0
> > >

Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[David Ahern]

On 11/15/24 1:46 PM, Sam Edwards wrote:
> Hi Hangbin,
> 
> It took me a while to grasp but the problem seems to be a confusion
> about what it means to set a temporary's lifetimes to 0/0:
> 1) "The mngtmpaddrs has gone away; this temporary is slated for
> deletion by addrconf_verify_rtnl()"
> 2) "This temporary address itself shall no longer be used, regenerate
> it immediately."
> 
> The existing behavior makes sense for the #2 case, but not for the #1
> case. It seems sensible to me to keep the #2 behavior as-is, because
> userspace might be setting a 0/0 lifetime to forcibly rotate the
> temporary.
> 
> So it sounds like (at least) one of three fixes is in order:
> a) Make ipv6_create_tempaddr() verify that the `ifp` is (still)
> alive+mngtmpaddrs, returning with an error code if not.
> b) Look at the 3 callsites for ipv6_create_tempaddr() and add the
> above verifications before calling.
> c) Add a function that calls ipv6_del_addr(temp) for every temporary
> with a specified ifpub, and use it instead of manage_tempaddrs(..., 0,
> 0, false, ...) when deleting/unflagging a mngtmpaddrs.
> 
> Personally I like option C the best. What are your thoughts?
> 
> Cheers,

Off the top of my head regarding recent changes, please include Maciej:

commit 69172f0bcb6a09110c5d2a6d792627f5095a9018
Author: Maciej Żenczykowski <maze@google.com>
Date:   Thu Jul 20 09:00:22 2023 -0700

    ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new
temporary address



and Alex in discussions around changes to temp addresses

commit f4bcbf360ac8dc424dc4d2b384b528e69b6f34d9
Author: Alex Henrie <alexhenrie24@gmail.com>
Date:   Tue Feb 13 23:26:32 2024 -0700

    net: ipv6/addrconf: clamp preferred_lft to the minimum required

Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

[Hangbin Liu]

On Fri, Nov 15, 2024 at 12:46:27PM -0800, Sam Edwards wrote:
> Hi Hangbin,
> 
> It took me a while to grasp but the problem seems to be a confusion
> about what it means to set a temporary's lifetimes to 0/0:
> 1) "The mngtmpaddrs has gone away; this temporary is slated for
> deletion by addrconf_verify_rtnl()"
> 2) "This temporary address itself shall no longer be used, regenerate
> it immediately."
> 
> The existing behavior makes sense for the #2 case, but not for the #1
> case. It seems sensible to me to keep the #2 behavior as-is, because
> userspace might be setting a 0/0 lifetime to forcibly rotate the
> temporary.
> 
> So it sounds like (at least) one of three fixes is in order:
> a) Make ipv6_create_tempaddr() verify that the `ifp` is (still)
> alive+mngtmpaddrs, returning with an error code if not.
> b) Look at the 3 callsites for ipv6_create_tempaddr() and add the
> above verifications before calling.
> c) Add a function that calls ipv6_del_addr(temp) for every temporary
> with a specified ifpub, and use it instead of manage_tempaddrs(..., 0,
> 0, false, ...) when deleting/unflagging a mngtmpaddrs.
> 
> Personally I like option C the best. What are your thoughts?

Hi Sam,

Thanks for the comments. I have no preference. Let me try option C
and update the test case first.

Hangbin

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

[Hangbin Liu]

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

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

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

[Jakub Kicinski]

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
-- 
pw-bot: cr

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

[Hangbin Liu]

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

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

[Jakub Kicinski]

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 ?

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

[Hangbin Liu]

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

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

[Jakub Kicinski]

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/
	....

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

[Hangbin Liu]

On Thu, Nov 14, 2024 at 07:13:40AM -0800, Jakub Kicinski wrote:
> > 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/
> 	....

I can't see how the pmtu test affects the dummy interface test in
a single namespace... I may add some debug info in the next version.

Thanks
Hangbin

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

[Sam Edwards]

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
>

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

[Hangbin Liu]

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

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

[Sam Edwards]

On Thu, Nov 14, 2024 at 12:46 AM Hangbin Liu <liuhangbin@gmail.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

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

[Hangbin Liu]

On Fri, Nov 15, 2024 at 12:59:27PM -0800, Sam Edwards wrote:
> 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`

Thanks, this is much clear.

> #
> # 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.

And I will modify and use your checking version.

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

Since we add all mngtmpaddrs manually, which valid/preferred_life_time
will be forever. So we only need to check the temporary addresses'
valid/preferred_life_time, right? And on the other hand, the preferred_lft
maybe 0 in my example.

    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

It looks we only need to check the valid_lft. Am I miss anything?

Thanks
Hangbin