Message-ID: <ZzWo5fJcraaDDLm_@fedora>
Date: Thu, 14 Nov 2024 07:38:13 +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,
	Maciej Żenczykowski <maze@...gle.com>,
	Xiao Ma <xiaom@...gle.com>
Subject: Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr
 is removed or un-mngtmpaddr

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

Powered by blists - more mailing lists