| Message ID | 20241113125152.752778-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv6: fix temporary address not removed correctly | expand |
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 >
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 --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();
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(+)