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