diff mbox series

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

Message ID 20241113125152.752778-2-liuhangbin@gmail.com (mailing list archive)
State New
Headers show
Series ipv6: fix temporary address not removed correctly | expand

Commit Message

Hangbin Liu Nov. 13, 2024, 12:51 p.m. UTC 
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(+)

Comments

Sam Edwards Nov. 13, 2024, 9:03 p.m. UTC | #1 
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
>
Hangbin Liu Nov. 14, 2024, 7:38 a.m. UTC | #2 
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
> >
diff mbox series

Patch

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