Message-ID: <CAH5Ym4hcguhXvJvVuANns7Q9VTOWR-SxHSdD55rR5BWhWeg2Ow@mail.gmail.com>
Date: Fri, 15 Nov 2024 12:46: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, 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 11:38 PM Hangbin Liu <liuhangbin@...il.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@...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()?

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

Powered by blists - more mailing lists