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(+)
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(+)
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();
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
- 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.
- 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.
- 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:
- "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:
- "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
Add a test to check the temporary address could be added/removed correctly when mngtempaddr is set or removed/unmanaged.
Suggested-by: Sam Edwards cfsworks@gmail.com Signed-off-by: Hangbin Liu liuhangbin@gmail.com --- tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh index bdf6f10d0558..f25a363d55bd 100755 --- a/tools/testing/selftests/net/rtnetlink.sh +++ b/tools/testing/selftests/net/rtnetlink.sh @@ -29,6 +29,7 @@ ALL_TESTS=" kci_test_bridge_parent_id kci_test_address_proto kci_test_enslave_bonding + kci_test_mngtmpaddr "
devdummy="test-dummy0" @@ -44,6 +45,7 @@ check_err() if [ $ret -eq 0 ]; then ret=$1 fi + [ -n "$2" ] && echo "$2" }
# same but inverted -- used when command must fail for test to pass @@ -1267,6 +1269,93 @@ kci_test_enslave_bonding() ip netns del "$testns" }
+# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting +check_tempaddr_exists() +{ + local start=${1-"1"} + addr_list=$(ip -j -n $testns addr show dev ${devdummy}) + for i in $(seq $start 4); do + if ! echo ${addr_list} | \ + jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \ + grep -q "200${i}"; then + check_err $? "No mngtmpaddr 200${i}:db8::1" + return 0 + fi + + if ! echo ${addr_list} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep -q "200${i}"; then + check_err $? "No tempaddr for 200${i}:db8::1" + return 0 + fi + done + return 1 +} + +kci_test_mngtmpaddr() +{ + local ret=0 + + setup_ns testns + if [ $? -ne 0 ]; then + end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns" + return $ksft_skip + fi + + # 1. Create a dummy Ethernet interface + run_cmd ip -n $testns link add ${devdummy} type dummy + run_cmd ip -n $testns link set ${devdummy} up + run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1 + # 2. Create several (3-4) mngtmpaddr addresses on that interface. + # with temp_*_lft configured to be pretty short (10 and 35 seconds + # for prefer/valid respectively) + for i in $(seq 1 4); do + run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr + tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep 200${i}) + #3. Confirm that temporary addresses are created immediately. + if [ -z $tempaddr ]; then + check_err 1 "no tempaddr created for 200${i}:db8::1" + else + run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \ + preferred_lft 10 valid_lft 35 + fi + done + + #4. Confirm that a preferred temporary address exists for each mngtmpaddr + # address at all times, polling once per second for at least 5 minutes. + slowwait 300 check_tempaddr_exists + + #5. Delete each mngtmpaddr address, one at a time (alternating between + # deleting and merely un-mngtmpaddr-ing), and confirm that the other + # mngtmpaddr addresses still have preferred temporaries. + for i in $(seq 1 4); do + if (( $i % 2 == 1 )); then + run_cmd ip -n $testns addr del 200${i}:db8::1/64 dev ${devdummy} + else + run_cmd ip -n $testns addr change 200${i}:db8::1/64 dev ${devdummy} + fi + # the temp addr should be deleted + if ip -j -n $testns addr show dev ${devdummy} | \ + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \ + grep -q "200${i}"; then + check_err 1 "tempaddr not deleted for 200${i}:db8::1" + fi + # Check other addresses are still exist + check_tempaddr_exists $((i + 1)) + done + + if [ $ret -ne 0 ]; then + end_test "FAIL: mngtmpaddr add/remove incorrect" + ip netns del "$testns" + return 1 + fi + + end_test "PASS: mngtmpaddr add/remove correctly" + ip netns del "$testns" +} + kci_test_rtnl() { local current_test
On Wed, 13 Nov 2024 12:51:52 +0000 Hangbin Liu wrote:
Add a test to check the temporary address could be added/removed correctly when mngtempaddr is set or removed/unmanaged.
Doesn't seem to work for us:
# [+300.25] tempaddr not deleted for 2001:db8::1 # [+0.16] tempaddr not deleted for 2003:db8::1 # [+0.07] FAIL: mngtmpaddr add/remove incorrect not ok 1 selftests: net: rtnetlink.sh # exit=1
On Wed, Nov 13, 2024 at 11:56:12AM -0800, Jakub Kicinski wrote:
On Wed, 13 Nov 2024 12:51:52 +0000 Hangbin Liu wrote:
Add a test to check the temporary address could be added/removed correctly when mngtempaddr is set or removed/unmanaged.
Doesn't seem to work for us:
# [+300.25] tempaddr not deleted for 2001:db8::1 # [+0.16] tempaddr not deleted for 2003:db8::1 # [+0.07] FAIL: mngtmpaddr add/remove incorrect not ok 1 selftests: net: rtnetlink.sh # exit=1
Is this tested with patched kernel or unpatched kernel. On my local side I got
# ./rtnetlink.sh -t kci_test_mngtmpaddr PASS: mngtmpaddr add/remove correctly
Thanks Hangbin
On Thu, 14 Nov 2024 02:00:01 +0000 Hangbin Liu wrote:
# [+300.25] tempaddr not deleted for 2001:db8::1 # [+0.16] tempaddr not deleted for 2003:db8::1 # [+0.07] FAIL: mngtmpaddr add/remove incorrect not ok 1 selftests: net: rtnetlink.sh # exit=1
Is this tested with patched kernel or unpatched kernel. On my local side I got
# ./rtnetlink.sh -t kci_test_mngtmpaddr PASS: mngtmpaddr add/remove correctly
I believe you that you run the test before sending. But if it doesn't pass in CI we can't merge this. Just a shot in the dark but does it also pass without the -t ?
On Wed, Nov 13, 2024 at 06:43:12PM -0800, Jakub Kicinski wrote:
On Thu, 14 Nov 2024 02:00:01 +0000 Hangbin Liu wrote:
# [+300.25] tempaddr not deleted for 2001:db8::1 # [+0.16] tempaddr not deleted for 2003:db8::1 # [+0.07] FAIL: mngtmpaddr add/remove incorrect not ok 1 selftests: net: rtnetlink.sh # exit=1
Is this tested with patched kernel or unpatched kernel. On my local side I got
# ./rtnetlink.sh -t kci_test_mngtmpaddr PASS: mngtmpaddr add/remove correctly
I believe you that you run the test before sending. But if it doesn't pass in CI we can't merge this. Just a shot in the dark but does it also pass without the -t ?
Yes. # ./rtnetlink.sh PASS: policy routing ... PASS: enslave interface in a bond PASS: mngtmpaddr add/remove correctly
I will modify the test with Sam's suggestions and see if it could pass.
Thanks Hangbin
On Thu, 14 Nov 2024 08:19:31 +0000 Hangbin Liu wrote:
Is this tested with patched kernel or unpatched kernel. On my local side I got
# ./rtnetlink.sh -t kci_test_mngtmpaddr PASS: mngtmpaddr add/remove correctly
I believe you that you run the test before sending. But if it doesn't pass in CI we can't merge this. Just a shot in the dark but does it also pass without the -t ?
Yes. # ./rtnetlink.sh PASS: policy routing ... PASS: enslave interface in a bond PASS: mngtmpaddr add/remove correctly
I will modify the test with Sam's suggestions and see if it could pass.
The other option is that some other test doesn't clean up after itself, since we reuse VMs. We have a script in NIPA to map things: contest/cithreadmap
Looks like we run rtnetlink.sh after pmtu.sh, I wonder if pmtu.sh is to blame:
Thread3-VM0 4-pmtu-sh/ 5-rtnetlink-sh/ ....
On Thu, Nov 14, 2024 at 07:13:40AM -0800, Jakub Kicinski wrote:
I will modify the test with Sam's suggestions and see if it could pass.
The other option is that some other test doesn't clean up after itself, since we reuse VMs. We have a script in NIPA to map things: contest/cithreadmap
Looks like we run rtnetlink.sh after pmtu.sh, I wonder if pmtu.sh is to blame:
Thread3-VM0 4-pmtu-sh/ 5-rtnetlink-sh/ ....
I can't see how the pmtu test affects the dummy interface test in a single namespace... I may add some debug info in the next version.
Thanks Hangbin
On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu liuhangbin@gmail.com wrote:
Add a test to check the temporary address could be added/removed correctly when mngtempaddr is set or removed/unmanaged.
Suggested-by: Sam Edwards cfsworks@gmail.com Signed-off-by: Hangbin Liu liuhangbin@gmail.com
Hi Hangbin,
Ahh, you beat me to it! I was starting to work on a test case of my own but this looks much better organized. :)
I have some comments in cases where our approaches differ:
tools/testing/selftests/net/rtnetlink.sh | 89 ++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh index bdf6f10d0558..f25a363d55bd 100755 --- a/tools/testing/selftests/net/rtnetlink.sh +++ b/tools/testing/selftests/net/rtnetlink.sh @@ -29,6 +29,7 @@ ALL_TESTS=" kci_test_bridge_parent_id kci_test_address_proto kci_test_enslave_bonding
kci_test_mngtmpaddr
"
devdummy="test-dummy0" @@ -44,6 +45,7 @@ check_err() if [ $ret -eq 0 ]; then ret=$1 fi
[ -n "$2" ] && echo "$2"
}
# same but inverted -- used when command must fail for test to pass @@ -1267,6 +1269,93 @@ kci_test_enslave_bonding() ip netns del "$testns" }
+# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting +check_tempaddr_exists() +{
local start=${1-"1"}
addr_list=$(ip -j -n $testns addr show dev ${devdummy})
for i in $(seq $start 4); do
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No mngtmpaddr 200${i}:db8::1"
return 0
fi
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No tempaddr for 200${i}:db8::1"
return 0
fi
done
return 1
+}
The variant of this function that I implemented is a lot less "fixed" and gathers all IPv6 prefixes (by /64) into one of 3 sets: 1. mngtmpaddr 2. temporary, not deprecated 3. temporary (whether deprecated or not)
It then ensures that set 3 is a subset of set 1, and set 1 is a subset of set 2. (And if it's easy: it should also ensure that no 'temporary' has a *_lft in excess of its parent's.)
Doing it this way allows the test case to create, modify, and delete mngtmpaddrs according to the needs of the test, and the check() function only ensures that the rules are being obeyed, it doesn't make assumptions about the expected state of the addresses.
+kci_test_mngtmpaddr() +{
local ret=0
setup_ns testns
if [ $? -ne 0 ]; then
end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns"
return $ksft_skip
fi
# 1. Create a dummy Ethernet interface
run_cmd ip -n $testns link add ${devdummy} type dummy
run_cmd ip -n $testns link set ${devdummy} up
run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1
Test should also set .temp_prefered_lft and .temp_valid_lft here.
I also set .max_desync_factor=0 because this is a dummy interface that doesn't have any latency, which allows the prefer lifetime to be pretty short. (See below.)
# 2. Create several (3-4) mngtmpaddr addresses on that interface.
# with temp_*_lft configured to be pretty short (10 and 35 seconds
# for prefer/valid respectively)
for i in $(seq 1 4); do
run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr
I don't really like using 200X:db8::1 as the test addresses. 2001:db8::/32 is the IANA designated prefix for examples/documentation (and, by extension, unit tests) so we should really try to remain inside that.
Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak) just to minimize the chances of conflicting with something else in the system. Though, with the test happening in its own netns, *that* level of caution may not be necessary.
Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there.
tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep 200${i})
#3. Confirm that temporary addresses are created immediately.
This could simply be a call to the above genericized check() function.
if [ -z $tempaddr ]; then
check_err 1 "no tempaddr created for 200${i}:db8::1"
else
run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \
preferred_lft 10 valid_lft 35
While Linux is (apparently) happy to let userspace modify the tempaddr's remaining lifetime like this, I don't think this is a common or recommended practice. Rather, the test should be letting Linux manage the tempaddr lifetimes and rotate the addresses itself.
fi
done
Here is a good place to create an address that *isn't* mngtmpaddr, confirm there is no temporary (via call to check() function), then add the `mngtmpaddr` flag after the fact.
#4. Confirm that a preferred temporary address exists for each mngtmpaddr
# address at all times, polling once per second for at least 5 minutes.
slowwait 300 check_tempaddr_exists
So I previously said "wait 5 minutes" but I later saw in the documentation for the selftest suite that maintainers really don't like it when a test takes more than ~45 seconds to run. We might want to drop this wait to 30 by default and accelerate the timetable on prefer/valid lifetimes to something like 10/25.
#5. Delete each mngtmpaddr address, one at a time (alternating between
# deleting and merely un-mngtmpaddr-ing), and confirm that the other
# mngtmpaddr addresses still have preferred temporaries.
for i in $(seq 1 4); do
if (( $i % 2 == 1 )); then
run_cmd ip -n $testns addr del 200${i}:db8::1/64 dev ${devdummy}
else
run_cmd ip -n $testns addr change 200${i}:db8::1/64 dev ${devdummy}
fi
# the temp addr should be deleted
if ip -j -n $testns addr show dev ${devdummy} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep -q "200${i}"; then
check_err 1 "tempaddr not deleted for 200${i}:db8::1"
fi
# Check other addresses are still exist
check_tempaddr_exists $((i + 1))
Ditto, the whole bottom half of this loop can be a call to a genericized check function.
done
if [ $ret -ne 0 ]; then
end_test "FAIL: mngtmpaddr add/remove incorrect"
ip netns del "$testns"
return 1
fi
end_test "PASS: mngtmpaddr add/remove correctly"
ip netns del "$testns"
Do we need to make sure the netns gets cleaned up via `trap ... EXIT` so that it doesn't leak if the user interrupts the test? Or does the greater test fixture take care of that for us?
Happy Wednesday, Sam
+}
kci_test_rtnl() { local current_test -- 2.46.0
Hi Sam,
On Wed, Nov 13, 2024 at 12:43:00PM -0800, Sam Edwards wrote:
+# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting +check_tempaddr_exists() +{
local start=${1-"1"}
addr_list=$(ip -j -n $testns addr show dev ${devdummy})
for i in $(seq $start 4); do
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No mngtmpaddr 200${i}:db8::1"
return 0
fi
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No tempaddr for 200${i}:db8::1"
return 0
fi
done
return 1
+}
The variant of this function that I implemented is a lot less "fixed" and gathers all IPv6 prefixes (by /64) into one of 3 sets:
- mngtmpaddr
- temporary, not deprecated
- temporary (whether deprecated or not)
It then ensures that set 3 is a subset of set 1, and set 1 is a subset of set 2. (And if it's easy: it should also ensure that no 'temporary' has a *_lft in excess of its parent's.)
I'm not totally get your explanation here. e.g. with preferred_lft 10, valid_lft 30. I got the following result.
# ip addr show dummy0 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000 link/ether 2e:f7:df:87:44:64 brd ff:ff:ff:ff:ff:ff inet6 2001::743:ec1e:5c19:404f/64 scope global temporary dynamic valid_lft 25sec preferred_lft 5sec inet6 2001::938f:432:f32d:602f/64 scope global temporary dynamic valid_lft 19sec preferred_lft 0sec inet6 2001::5b65:c0a3:cd8c:edf8/64 scope global temporary deprecated dynamic valid_lft 3sec preferred_lft 0sec inet6 2001::8a7e:6e8d:83f1:9ea0/64 scope global temporary deprecated dynamic valid_lft 0sec preferred_lft 0sec inet6 2001::1/64 scope global mngtmpaddr valid_lft forever preferred_lft forever
So there are 1 mngtmpaddr, 2 temporary address (not deprecated). 4 total temporary address. Based on your rule. It should be set 1 is a subset of set 2. Set 2 is a subset of 3.
And how do we ensure that no 'temporary' has a *_lft in excess of its parent's.
Doing it this way allows the test case to create, modify, and delete mngtmpaddrs according to the needs of the test, and the check() function only ensures that the rules are being obeyed, it doesn't make assumptions about the expected state of the addresses.
I'm not sure if this is totally enough. What if there are 3 mngtmpaddrs and 4 temporary address. But actually 1 mngtmpaddrs doesn't have temporary address. Maybe check() needs to check only 1 prefix each time.
+kci_test_mngtmpaddr() +{
local ret=0
setup_ns testns
if [ $? -ne 0 ]; then
end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns"
return $ksft_skip
fi
# 1. Create a dummy Ethernet interface
run_cmd ip -n $testns link add ${devdummy} type dummy
run_cmd ip -n $testns link set ${devdummy} up
run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1
Test should also set .temp_prefered_lft and .temp_valid_lft here.
I also set .max_desync_factor=0 because this is a dummy interface that doesn't have any latency, which allows the prefer lifetime to be pretty short. (See below.)
Thanks, I will fix the test.
# 2. Create several (3-4) mngtmpaddr addresses on that interface.
# with temp_*_lft configured to be pretty short (10 and 35 seconds
# for prefer/valid respectively)
for i in $(seq 1 4); do
run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr
I don't really like using 200X:db8::1 as the test addresses. 2001:db8::/32 is the IANA designated prefix for examples/documentation (and, by extension, unit tests) so we should really try to remain inside that.
Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak) just to minimize the chances of conflicting with something else in the system. Though, with the test happening in its own netns, *that* level of caution may not be necessary.
Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there.
OK, I will use 2001:db8::/32 for testing.
tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep 200${i})
#3. Confirm that temporary addresses are created immediately.
This could simply be a call to the above genericized check() function.
if [ -z $tempaddr ]; then
check_err 1 "no tempaddr created for 200${i}:db8::1"
else
run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \
preferred_lft 10 valid_lft 35
While Linux is (apparently) happy to let userspace modify the tempaddr's remaining lifetime like this, I don't think this is a common or recommended practice. Rather, the test should be letting Linux manage the tempaddr lifetimes and rotate the addresses itself.
OK
fi
done
Here is a good place to create an address that *isn't* mngtmpaddr, confirm there is no temporary (via call to check() function), then add the `mngtmpaddr` flag after the fact.
OK, I will
#4. Confirm that a preferred temporary address exists for each mngtmpaddr
# address at all times, polling once per second for at least 5 minutes.
slowwait 300 check_tempaddr_exists
So I previously said "wait 5 minutes" but I later saw in the documentation for the selftest suite that maintainers really don't like it when a test takes more than ~45 seconds to run. We might want to drop this wait to 30 by default and accelerate the timetable on prefer/valid lifetimes to something like 10/25.
Yes, 5m is too long for a single test.
end_test "PASS: mngtmpaddr add/remove correctly"
ip netns del "$testns"
Do we need to make sure the netns gets cleaned up via `trap ... EXIT` so that it doesn't leak if the user interrupts the test? Or does the greater test fixture take care of that for us?
No, rtnetlink.sh doesn't have a trap function. I plan to add the trap function separately.
Thanks Hangbin
On Thu, Nov 14, 2024 at 12:46 AM Hangbin Liu liuhangbin@gmail.com wrote:
Hi Sam,
On Wed, Nov 13, 2024 at 12:43:00PM -0800, Sam Edwards wrote:
+# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting +check_tempaddr_exists() +{
local start=${1-"1"}
addr_list=$(ip -j -n $testns addr show dev ${devdummy})
for i in $(seq $start 4); do
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No mngtmpaddr 200${i}:db8::1"
return 0
fi
if ! echo ${addr_list} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep -q "200${i}"; then
check_err $? "No tempaddr for 200${i}:db8::1"
return 0
fi
done
return 1
+}
The variant of this function that I implemented is a lot less "fixed" and gathers all IPv6 prefixes (by /64) into one of 3 sets:
- mngtmpaddr
- temporary, not deprecated
- temporary (whether deprecated or not)
It then ensures that set 3 is a subset of set 1, and set 1 is a subset of set 2. (And if it's easy: it should also ensure that no 'temporary' has a *_lft in excess of its parent's.)
I'm not totally get your explanation here. e.g. with preferred_lft 10, valid_lft 30. I got the following result.
# ip addr show dummy0 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000 link/ether 2e:f7:df:87:44:64 brd ff:ff:ff:ff:ff:ff inet6 2001::743:ec1e:5c19:404f/64 scope global temporary dynamic valid_lft 25sec preferred_lft 5sec inet6 2001::938f:432:f32d:602f/64 scope global temporary dynamic valid_lft 19sec preferred_lft 0sec inet6 2001::5b65:c0a3:cd8c:edf8/64 scope global temporary deprecated dynamic valid_lft 3sec preferred_lft 0sec inet6 2001::8a7e:6e8d:83f1:9ea0/64 scope global temporary deprecated dynamic valid_lft 0sec preferred_lft 0sec inet6 2001::1/64 scope global mngtmpaddr valid_lft forever preferred_lft forever
So there are 1 mngtmpaddr, 2 temporary address (not deprecated). 4 total temporary address. Based on your rule. It should be set 1 is a subset of set 2. Set 2 is a subset of 3.
And how do we ensure that no 'temporary' has a *_lft in excess of its parent's.
Doing it this way allows the test case to create, modify, and delete mngtmpaddrs according to the needs of the test, and the check() function only ensures that the rules are being obeyed, it doesn't make assumptions about the expected state of the addresses.
I'm not sure if this is totally enough. What if there are 3 mngtmpaddrs and 4 temporary address. But actually 1 mngtmpaddrs doesn't have temporary address. Maybe check() needs to check only 1 prefix each time.
Hi Hangbin,
My apologies, I should have shared my version of the check function before. Here it is:
```bash # Called to validate the addresses on $IFNAME: # # 1. Every `temporary` address must have a matching `mngtmpaddr` # 2. Every `mngtmpaddr` address must have some un`deprecated` `temporary` # # Fails the whole test script if a problem is detected, else returns silently. validate() { mng_prefixes=() undep_prefixes=() temp_addrs=()
while read -r line; do line_array=($line) address="${line_array[1]}" prefix="$(echo "$address" | cut -d: -f1-4)::/64" if echo "$line" | grep -q mngtmpaddr; then mng_prefixes+=("$prefix") elif echo "$line" | grep -q temporary; then temp_addrs+=("$address") if echo "$line" | grep -qv deprecated; then undep_prefixes+=("$prefix") fi fi done < <(ip -6 addr show dev $IFNAME | grep '^\s*inet6')
# 1. All temporary addresses (temp and dep) must have a matching mngtmpaddr for address in ${temp_addrs[@]}; do prefix="$(echo "$address" | cut -d: -f1-4)::/64" if [[ ! " ${mng_prefixes[*]} " =~ " $prefix " ]]; then echo "FAIL: Temporary $address with no matching mngtmpaddr!"; exit 1 fi done
# 2. All mngtmpaddr addresses must have a temporary address (not dep) for prefix in ${mng_prefixes[@]}; do if [[ ! " ${undep_prefixes[*]} " =~ " $prefix " ]]; then echo "FAIL: No undeprecated temporary in $prefix!"; exit 1 fi done } ```
Of course this is using awful text parsing and not JSON output. But the idea is that it groups addresses by their /64 prefix, to confirm that every /64 containing a mngtmpaddrs address also contains an undeprecated temporary, and that every /64 containing a temporary (deprecated or not) contains a mngtmpaddrs.
This can be extended for the lifetime checking: when we build the set of mngtmpaddrs /64s, we also note the valid/preferred_life_time values for each mngtmpaddr. Then later when we confirm rule 1 (all temporary addresses must have a matching mngtmpaddr) we also confirm that each temporary does not outlive the mngtmpaddr in the same /64.
Happy Friday, Sam
+kci_test_mngtmpaddr() +{
local ret=0
setup_ns testns
if [ $? -ne 0 ]; then
end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns"
return $ksft_skip
fi
# 1. Create a dummy Ethernet interface
run_cmd ip -n $testns link add ${devdummy} type dummy
run_cmd ip -n $testns link set ${devdummy} up
run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1
Test should also set .temp_prefered_lft and .temp_valid_lft here.
I also set .max_desync_factor=0 because this is a dummy interface that doesn't have any latency, which allows the prefer lifetime to be pretty short. (See below.)
Thanks, I will fix the test.
# 2. Create several (3-4) mngtmpaddr addresses on that interface.
# with temp_*_lft configured to be pretty short (10 and 35 seconds
# for prefer/valid respectively)
for i in $(seq 1 4); do
run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr
I don't really like using 200X:db8::1 as the test addresses. 2001:db8::/32 is the IANA designated prefix for examples/documentation (and, by extension, unit tests) so we should really try to remain inside that.
Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak) just to minimize the chances of conflicting with something else in the system. Though, with the test happening in its own netns, *that* level of caution may not be necessary.
Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there.
OK, I will use 2001:db8::/32 for testing.
tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \
jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
grep 200${i})
#3. Confirm that temporary addresses are created immediately.
This could simply be a call to the above genericized check() function.
if [ -z $tempaddr ]; then
check_err 1 "no tempaddr created for 200${i}:db8::1"
else
run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \
preferred_lft 10 valid_lft 35
While Linux is (apparently) happy to let userspace modify the tempaddr's remaining lifetime like this, I don't think this is a common or recommended practice. Rather, the test should be letting Linux manage the tempaddr lifetimes and rotate the addresses itself.
OK
fi
done
Here is a good place to create an address that *isn't* mngtmpaddr, confirm there is no temporary (via call to check() function), then add the `mngtmpaddr` flag after the fact.
OK, I will
#4. Confirm that a preferred temporary address exists for each mngtmpaddr
# address at all times, polling once per second for at least 5 minutes.
slowwait 300 check_tempaddr_exists
So I previously said "wait 5 minutes" but I later saw in the documentation for the selftest suite that maintainers really don't like it when a test takes more than ~45 seconds to run. We might want to drop this wait to 30 by default and accelerate the timetable on prefer/valid lifetimes to something like 10/25.
Yes, 5m is too long for a single test.
end_test "PASS: mngtmpaddr add/remove correctly"
ip netns del "$testns"
Do we need to make sure the netns gets cleaned up via `trap ... EXIT` so that it doesn't leak if the user interrupts the test? Or does the greater test fixture take care of that for us?
No, rtnetlink.sh doesn't have a trap function. I plan to add the trap function separately.
Thanks Hangbin
On Fri, Nov 15, 2024 at 12:59:27PM -0800, Sam Edwards wrote:
Hi Hangbin,
My apologies, I should have shared my version of the check function before. Here it is:
# Called to validate the addresses on $IFNAME: # # 1. Every `temporary` address must have a matching `mngtmpaddr` # 2. Every `mngtmpaddr` address must have some un`deprecated` `temporary`
Thanks, this is much clear.
# # Fails the whole test script if a problem is detected, else returns silently. validate() { mng_prefixes=() undep_prefixes=() temp_addrs=()
while read -r line; do line_array=($line) address="${line_array[1]}" prefix="$(echo "$address" | cut -d: -f1-4)::/64" if echo "$line" | grep -q mngtmpaddr; then mng_prefixes+=("$prefix") elif echo "$line" | grep -q temporary; then temp_addrs+=("$address") if echo "$line" | grep -qv deprecated; then undep_prefixes+=("$prefix") fi fi done < <(ip -6 addr show dev $IFNAME | grep '^\s*inet6') # 1. All temporary addresses (temp and dep) must have a matching mngtmpaddr for address in ${temp_addrs[@]}; do prefix="$(echo "$address" | cut -d: -f1-4)::/64" if [[ ! " ${mng_prefixes[*]} " =~ " $prefix " ]]; then echo "FAIL: Temporary $address with no matching mngtmpaddr!"; exit 1 fi done # 2. All mngtmpaddr addresses must have a temporary address (not dep) for prefix in ${mng_prefixes[@]}; do if [[ ! " ${undep_prefixes[*]} " =~ " $prefix " ]]; then echo "FAIL: No undeprecated temporary in $prefix!"; exit 1 fi done
}
Of course this is using awful text parsing and not JSON output. But the idea is that it groups addresses by their /64 prefix, to confirm that every /64 containing a mngtmpaddrs address also contains an undeprecated temporary, and that every /64 containing a temporary (deprecated or not) contains a mngtmpaddrs.
And I will modify and use your checking version.
This can be extended for the lifetime checking: when we build the set of mngtmpaddrs /64s, we also note the valid/preferred_life_time values for each mngtmpaddr. Then later when we confirm rule 1 (all temporary addresses must have a matching mngtmpaddr) we also confirm that each temporary does not outlive the mngtmpaddr in the same /64.
Since we add all mngtmpaddrs manually, which valid/preferred_life_time will be forever. So we only need to check the temporary addresses' valid/preferred_life_time, right? And on the other hand, the preferred_lft maybe 0 in my example.
inet6 2001::743:ec1e:5c19:404f/64 scope global temporary dynamic valid_lft 25sec preferred_lft 5sec inet6 2001::938f:432:f32d:602f/64 scope global temporary dynamic valid_lft 19sec preferred_lft 0sec
It looks we only need to check the valid_lft. Am I miss anything?
Thanks Hangbin
linux-kselftest-mirror@lists.linaro.org