| Message ID | 20241113125152.752778-1-liuhangbin@gmail.com |
|---|---|
| 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 > >
On Wed, Nov 13, 2024 at 11:38 PM Hangbin Liu <liuhangbin@gmail.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@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()? 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 > > >
On 11/15/24 1:46 PM, Sam Edwards wrote: > 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, Off the top of my head regarding recent changes, please include Maciej: commit 69172f0bcb6a09110c5d2a6d792627f5095a9018 Author: Maciej Żenczykowski <maze@google.com> Date: Thu Jul 20 09:00:22 2023 -0700 ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address and Alex in discussions around changes to temp addresses commit f4bcbf360ac8dc424dc4d2b384b528e69b6f34d9 Author: Alex Henrie <alexhenrie24@gmail.com> Date: Tue Feb 13 23:26:32 2024 -0700 net: ipv6/addrconf: clamp preferred_lft to the minimum required
On Fri, Nov 15, 2024 at 12:46:27PM -0800, Sam Edwards wrote: > 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? Hi Sam, Thanks for the comments. I have no preference. Let me try option C and update the test case first. Hangbin
Currently the temporary address is not removed when mngtmpaddr is deleted or becomes unmanaged. The patch set fixed this issue and add a related test. Hangbin Liu (2): net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr selftests/rtnetlink.sh: add mngtempaddr test net/ipv6/addrconf.c | 5 ++ tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++ 2 files changed, 94 insertions(+)