ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

PATCH Superseded Maciej Żenczykowski 6 participants 19 messages Jul 12, 2023 – Jul 25, 2023

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Maciej Żenczykowski

author

2023-07-12 13:55 UTC #0 lore raw

PATCH 1/1 ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::44f3:581c:8ca:3983/64 scope global temporary dynamic
         valid_lft 604800sec preferred_lft 86172sec
      inet6 2000::1/64 scope global mngtmpaddr
         valid_lft forever preferred_lft forever
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

  # ip -6 addr del 2000::44f3:581c:8ca:3983/64 dev dummy1

(can wait a few seconds if you want to, the above delete isn't [directly] the problem)

  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
inet6 2000::1/64 scope global mngtmpaddr
valid_lft forever preferred_lft forever
inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
valid_lft forever preferred_lft forever

  # ip -6 addr del 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::81c9:56b7:f51a:b98f/64 scope global temporary dynamic
         valid_lft 604797sec preferred_lft 86169sec
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

This patch prevents this new 'global temporary dynamic' address from being created by the deletion of the related (same subnet prefix) 'mngtmpaddr' (which is triggered by there already being no temporary addresses).

Cc "David S. Miller" <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Pirko <[email protected]>

Fixes 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

net/ipv6/addrconf.c +10 -4

index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
 			ipv6_ifa_notify(0, ift);
 	}
 
-	if ((create || list_empty(&idev->tempaddr_list)) &&
-	    idev->cnf.use_tempaddr > 0) {
+	/* Also create a temporary address if it's enabled but no temporary
+	 * address currently exists.
+	 * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+	 * as part of cleanup (ie. deleting the mngtmpaddr).
+	 * We don't want that to result in creating a new temporary ip address.
+	 */
+	if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+		create = true;
+
+	if (create && idev->cnf.use_tempaddr > 0) {
 		/* When a new public address is created as described
 		 * in [ADDRCONF], also create a new temporary address.
-		 * Also create a temporary address if it's enabled but
-		 * no temporary address currently exists.
 		 */
 		read_unlock_bh(&idev->lock);
 		ipv6_create_tempaddr(ifp, false);

Signature · 1 line

2.41.0.255.g8b1d071c50-goog

Maciej Żenczykowski

2023-07-12 13:59 UTC #1 lore raw

Quoted 78 lines · Maciej Żenczykowski

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::44f3:581c:8ca:3983/64 scope global temporary dynamic
         valid_lft 604800sec preferred_lft 86172sec
      inet6 2000::1/64 scope global mngtmpaddr
         valid_lft forever preferred_lft forever
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

  # ip -6 addr del 2000::44f3:581c:8ca:3983/64 dev dummy1

(can wait a few seconds if you want to, the above delete isn't [directly] the problem)

  # ip -6 addr show dev dummy1

  # ip -6 addr del 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::81c9:56b7:f51a:b98f/64 scope global temporary dynamic
         valid_lft 604797sec preferred_lft 86169sec
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

Cc "David S. Miller" <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Pirko <[email protected]>

Fixes 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

net/ipv6/addrconf.c +10 -5

index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
                        ipv6_ifa_notify(0, ift);
        }
 
-       if ((create || list_empty(&idev->tempaddr_list)) &&
-           idev->cnf.use_tempaddr > 0) {
+       /* Also create a temporary address if it's enabled but no temporary
+        * address currently exists.
+        * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+        * as part of cleanup (ie. deleting the mngtmpaddr).
+        * We don't want that to result in creating a new temporary ip address.
+        */
+       if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+               create = true;
+
+       if (create && idev->cnf.use_tempaddr > 0) {
                /* When a new public address is created as described
                 * in [ADDRCONF], also create a new temporary address.
-                * Also create a temporary address if it's enabled but
-                * no temporary address currently exists.
                 */
                read_unlock_bh(&idev->lock);
                ipv6_create_tempaddr(ifp, false);
--

Quoted 2 lines

2.41.0.255.g8b1d071c50-goog

eh, should have included:

Reported-by Xiao Ma <[email protected]>

David Ahern

maintainer

2023-07-13 14:59 UTC #2 lore raw

In reply to Maciej Żenczykowski

Quoted 8 lines · Maciej Żenczykowski

net/ipv6/addrconf.c +0 -0

index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
 			ipv6_ifa_notify(0, ift);
 	}

net/ipv6/addrconf.c static void manage_tempaddrs(struct inet6_dev *idev, · line 2571

Quoted 10 lines

net/ipv6/addrconf.c +8 -2

-	if ((create || list_empty(&idev->tempaddr_list)) &&
-	    idev->cnf.use_tempaddr > 0) {
+	/* Also create a temporary address if it's enabled but no temporary
+	 * address currently exists.
+	 * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+	 * as part of cleanup (ie. deleting the mngtmpaddr).
+	 * We don't want that to result in creating a new temporary ip address.
+	 */
+	if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+		create = true;

I am not so sure about this part. manage_tempaddrs has 4 callers -- autoconf (prefix receive), address add, address modify and address delete. Seems like all of them have 'create' set properly when an address is wanted in which case maybe the answer here is don't let empty address list override create.

Quoted 9 lines

net/ipv6/addrconf.c +2 -2

+
+	if (create && idev->cnf.use_tempaddr > 0) {
 		/* When a new public address is created as described
 		 * in [ADDRCONF], also create a new temporary address.
-		 * Also create a temporary address if it's enabled but
-		 * no temporary address currently exists.
 		 */
 		read_unlock_bh(&idev->lock);
 		ipv6_create_tempaddr(ifp, false);

Maciej Żenczykowski

author

2023-07-13 15:03 UTC #3 lore raw

Quoted 10 lines · David Ahern

Quoted 8 lines · Maciej Żenczykowski

net/ipv6/addrconf.c +0 -0

index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
                      ipv6_ifa_notify(0, ift);
      }

Quoted 16 lines

net/ipv6/addrconf.c static void manage_tempaddrs(struct inet6_dev *idev, · line 2571
Quoted 10 lines
net/ipv6/addrconf.c +8 -2
-     if ((create || list_empty(&idev->tempaddr_list)) &&
-         idev->cnf.use_tempaddr > 0) {
+     /* Also create a temporary address if it's enabled but no temporary
+      * address currently exists.
+      * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+      * as part of cleanup (ie. deleting the mngtmpaddr).
+      * We don't want that to result in creating a new temporary ip address.
+      */
+     if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+             create = true;
I am not so sure about this part. manage_tempaddrs has 4 callers -- autoconf (prefix receive), address add, address modify and address delete. Seems like all of them have 'create' set properly when an address is wanted in which case maybe the answer here is don't let empty address list override create.

I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).

Are you convinced dropping the || list_empty would work? I assume it's there for some reason...

David Ahern

maintainer

2023-07-14 14:49 UTC #4 lore raw

Quoted 28 lines · Maciej Żenczykowski

Quoted 10 lines · David Ahern
Quoted 8 lines · Maciej Żenczykowski
net/ipv6/addrconf.c +0 -0
index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
                      ipv6_ifa_notify(0, ift);
      }
 
Quoted 16 lines
net/ipv6/addrconf.c static void manage_tempaddrs(struct inet6_dev *idev, · line 2571
Quoted 10 lines
net/ipv6/addrconf.c +8 -2
-     if ((create || list_empty(&idev->tempaddr_list)) &&
-         idev->cnf.use_tempaddr > 0) {
+     /* Also create a temporary address if it's enabled but no temporary
+      * address currently exists.
+      * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+      * as part of cleanup (ie. deleting the mngtmpaddr).
+      * We don't want that to result in creating a new temporary ip address.
+      */
+     if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+             create = true;
I am not so sure about this part. manage_tempaddrs has 4 callers -- autoconf (prefix receive), address add, address modify and address delete. Seems like all of them have 'create' set properly when an address is wanted in which case maybe the answer here is don't let empty address list override create.

Quoted 6 lines

I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...

I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.

Jakub Kicinski

maintainer

2023-07-18 23:08 UTC #5 lore raw

Quoted 9 lines · On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:

Quoted 6 lines
I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...
I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.

Did we get stuck here? Jiri are you around to answer?

David Ahern

maintainer

2023-07-18 23:18 UTC #6 lore raw

Quoted 5 lines · Jakub Kicinski

Quoted 4 lines · On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
Quoted 4 lines
I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...
I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.
Did we get stuck here? Jiri are you around to answer?

If Jiri is not around or does not remember, I suggest moving forward by dropping the list_empty check. Adding an address when one is deleted is not intuitive at all.

Jiri Pirko

2023-07-19 06:47 UTC #7 lore raw

In reply to Jakub Kicinski

Quoted 5 lines · Wed, Jul 19, 2023 at 01:08:32AM CEST, [email protected] wrote:

Quoted 4 lines · On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
Quoted 4 lines
I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...
I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.
Did we get stuck here? Jiri are you around to answer?

Most probably a bug. But, this is 10 years since the change, I don't remember much after this period. I didn't touch the code since :/

Maciej Żenczykowski

author

2023-07-19 12:51 UTC #8 lore raw

Quoted 9 lines · Jiri Pirko

Quoted 8 lines · Wed, Jul 19, 2023 at 01:08:32AM CEST, [email protected] wrote:
Quoted 7 lines · On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
Quoted 6 lines
I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.
Did we get stuck here? Jiri are you around to answer?
Most probably a bug. But, this is 10 years since the change, I don't remember much after this period. I didn't touch the code since :/

I *think* there might be cases where we want
addrconf_prefix_rcv_add_addr() -> manage_tempaddrs(create=false)
to result in the creation of a new temporary address.

Basically the 'create' argument is a boolean with interpretation "was managetmpaddr added/created" as opposed to "should a temporary address be created"

Think:
- RA comes in, we create the managetmpaddr, we call manage_tempaddrs(create=true), a temporary address gets created
- someone comes in and deletes the temporary address (perhaps by hand? or it expires?)
- another RA comes in, we don't create the managetmpaddr, since it already exists, we call manage_tempaddrs(create=false),
it notices there are no temporary addresses (by virtue of the ||
list_empty check), and creates a new one.

Note that:
#define TEMP_VALID_LIFETIME (7*86400)
#define TEMP_PREFERRED_LIFETIME (86400)
but these are tweakable...

  $ cat /proc/sys/net/ipv6/conf/*/temp_valid_lft | uniq -c
     37 604800
  $ cat /proc/sys/net/ipv6/conf/*/temp_prefered_lft | uniq -c
     37 86400
so we could have these be < unsolicited RA frequency.
(that's probably a bad idea for other reasons... but that's besides the point)

I have similar misgivings about inet6_addr_modify() -> manage_tempaddrs()

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

Here create == !was_managetempaddr,
but technically we can have create == false, and yet valid/prefered != 0.

This will be the case if we have inet6_addr_modify() called where *both* the before and after state is a managetempaddr. Perhaps because the expiration was updated?

Anyway... because of the above I remain unconvinced that just removing '|| list_empty' is safe...

David Ahern

maintainer

2023-07-20 15:35 UTC #9 lore raw

Quoted 63 lines · Maciej Żenczykowski

Quoted 9 lines · Jiri Pirko
Quoted 8 lines · Wed, Jul 19, 2023 at 01:08:32AM CEST, [email protected] wrote:
Quoted 7 lines · On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
Quoted 6 lines
I did consider that and I couldn't quite convince myself that simply removing "|| list_empty()" from the if statement is necessarily correct (thus I went with the more obviously correct change).
Are you convinced dropping the || list_empty would work? I assume it's there for some reason...
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I am hoping Jiri can recall why that part was added since it has the side effect of adding an address on a delete which should not happen.
Did we get stuck here? Jiri are you around to answer?
Most probably a bug. But, this is 10 years since the change, I don't remember much after this period. I didn't touch the code since :/
I *think* there might be cases where we want
addrconf_prefix_rcv_add_addr() -> manage_tempaddrs(create=false)
to result in the creation of a new temporary address.
Basically the 'create' argument is a boolean with interpretation "was managetmpaddr added/created" as opposed to "should a temporary address be created"
Think:
- RA comes in, we create the managetmpaddr, we call manage_tempaddrs(create=true), a temporary address gets created
- someone comes in and deletes the temporary address (perhaps by hand? or it expires?)
- another RA comes in, we don't create the managetmpaddr, since it already exists, we call manage_tempaddrs(create=false),
it notices there are no temporary addresses (by virtue of the ||
list_empty check), and creates a new one.
Note that:
#define TEMP_VALID_LIFETIME (7*86400)
#define TEMP_PREFERRED_LIFETIME (86400)
but these are tweakable...
  $ cat /proc/sys/net/ipv6/conf/*/temp_valid_lft | uniq -c
     37 604800
  $ cat /proc/sys/net/ipv6/conf/*/temp_prefered_lft | uniq -c
     37 86400
so we could have these be < unsolicited RA frequency.
(that's probably a bad idea for other reasons... but that's besides the point)
I have similar misgivings about inet6_addr_modify() -> manage_tempaddrs()
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);
}
Here create == !was_managetempaddr,
but technically we can have create == false, and yet valid/prefered != 0.

Quoted 6 lines

This will be the case if we have inet6_addr_modify() called where *both* the before and after state is a managetempaddr. Perhaps because the expiration was updated?
Anyway... because of the above I remain unconvinced that just removing '|| list_empty' is safe...

ok, want to resend this patch?

Maciej Żenczykowski

author

2023-07-20 16:00 UTC #10 lore raw

PATCH v2 1/1 ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::44f3:581c:8ca:3983/64 scope global temporary dynamic
         valid_lft 604800sec preferred_lft 86172sec
      inet6 2000::1/64 scope global mngtmpaddr
         valid_lft forever preferred_lft forever
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

  # ip -6 addr del 2000::44f3:581c:8ca:3983/64 dev dummy1

(can wait a few seconds if you want to, the above delete isn't [directly] the problem)

  # ip -6 addr show dev dummy1

  # ip -6 addr del 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000

      inet6 2000::81c9:56b7:f51a:b98f/64 scope global temporary dynamic
         valid_lft 604797sec preferred_lft 86169sec
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever

Cc "David S. Miller" <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Pirko <[email protected]>

Fixes 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")

Reported-by Xiao Ma <[email protected]>

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

net/ipv6/addrconf.c +10 -4

index e5213e598a04..94cec2075eee 100644
@@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
 			ipv6_ifa_notify(0, ift);
 	}
 
-	if ((create || list_empty(&idev->tempaddr_list)) &&
-	    idev->cnf.use_tempaddr > 0) {
+	/* Also create a temporary address if it's enabled but no temporary
+	 * address currently exists.
+	 * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
+	 * as part of cleanup (ie. deleting the mngtmpaddr).
+	 * We don't want that to result in creating a new temporary ip address.
+	 */
+	if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
+		create = true;
+
+	if (create && idev->cnf.use_tempaddr > 0) {
 		/* When a new public address is created as described
 		 * in [ADDRCONF], also create a new temporary address.
-		 * Also create a temporary address if it's enabled but
-		 * no temporary address currently exists.
 		 */
 		read_unlock_bh(&idev->lock);
 		ipv6_create_tempaddr(ifp, false);

Signature · 1 line

2.41.0.255.g8b1d071c50-goog

Jakub Kicinski

maintainer

2023-07-20 16:34 UTC #11 lore raw

Quoted 7 lines · On Thu, 20 Jul 2023 09:00:22 -0700 Maciej Żenczykowski wrote:

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

FTR resending the patch as part of the same thread is really counter productive for the maintainers. We review patches in order, no MUA I know can be told to order things correctly when new versions are sent in reply.

Don't try too hard, be normal, please.

Maciej Żenczykowski

author

2023-07-20 16:53 UTC #12 lore raw

Quoted 10 lines · Jakub Kicinski

Quoted 7 lines · On Thu, 20 Jul 2023 09:00:22 -0700 Maciej Żenczykowski wrote:
currently on 6.4 net/main:
  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

Quoted 4 lines

FTR resending the patch as part of the same thread is really counter productive for the maintainers. We review patches in order, no MUA I know can be told to order things correctly when new versions are sent in reply.

Sorry, but I'm afraid as a non-maintainer I have no idea what your work flows are like.
(and it shows up just fine on patchwork.kernel.org which is what I thought you all used now...)

Quoted 1 line

Don't try too hard, be normal, please.

What's considered 'normal' seems to depend on the list.

I'm pretty sure I've been told to do --in-reply follow ups previously when I didn't
(though it might not have been on netdev...).

Email is really a very poor medium for code reviews in general.

Maciej Żenczykowski

author

2023-07-24 12:07 UTC #13 lore raw

I see this is once again marked as changes requested. Ok, I get it, you win.

Quoted 31 lines · Maciej Żenczykowski

Quoted 10 lines · Jakub Kicinski
Quoted 7 lines · On Thu, 20 Jul 2023 09:00:22 -0700 Maciej Żenczykowski wrote:
currently on 6.4 net/main:
  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1
Quoted 4 lines
FTR resending the patch as part of the same thread is really counter productive for the maintainers. We review patches in order, no MUA I know can be told to order things correctly when new versions are sent in reply.
Sorry, but I'm afraid as a non-maintainer I have no idea what your work flows are like.
(and it shows up just fine on patchwork.kernel.org which is what I thought you all used now...)
Quoted 1 line
Don't try too hard, be normal, please.
What's considered 'normal' seems to depend on the list.
I'm pretty sure I've been told to do --in-reply follow ups previously when I didn't
(though it might not have been on netdev...).
Email is really a very poor medium for code reviews in general.Maciej Żenczykowski, Kernel Networking Developer @ Google

Jakub Kicinski

maintainer

2023-07-24 18:09 UTC #14 lore raw

Quoted 2 lines · On Mon, 24 Jul 2023 14:07:06 +0200 Maciej Żenczykowski wrote:

I see this is once again marked as changes requested. Ok, I get it, you win.

FTR wasn't me who set it to changes requested :S
My comments were meant more as a request for future postings.

I don't see a repost so let's just bring the v2 back:

pw-bot: under review

Maciej Żenczykowski

author

2023-07-24 18:21 UTC #15 lore raw

Quoted 6 lines · Jakub Kicinski

Quoted 2 lines · On Mon, 24 Jul 2023 14:07:06 +0200 Maciej Żenczykowski wrote:
I see this is once again marked as changes requested. Ok, I get it, you win.
FTR wasn't me who set it to changes requested :S

That is actually part of the problem, I've commented on this in the past.

The transitions (for example from New -> Changes Requested) happen:
- with no notification (via email or otherwise), thus requiring periodic manual monitoring of patchworks web ui (is there a better way?)
- with no idea who made the change
- with no idea what 'changes' were actually requested (sure sometimes, this is indeed obvious, but not in this case for example).

Quoted 1 line

My comments were meant more as a request for future postings.

Ack, that's what I assumed. Sorry for the mess.

Quoted 1 line

I don't see a repost so let's just bring the v2 back:

Thanks!

David Ahern

maintainer Reviewed-by on v2 1/1

2023-07-24 18:26 UTC #16 lore raw

In reply to Maciej Żenczykowski

Quoted 47 lines · Maciej Żenczykowski

currently on 6.4 net/main:
  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1
11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
      inet6 2000::44f3:581c:8ca:3983/64 scope global temporary dynamic
         valid_lft 604800sec preferred_lft 86172sec
      inet6 2000::1/64 scope global mngtmpaddr
         valid_lft forever preferred_lft forever
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever
  # ip -6 addr del 2000::44f3:581c:8ca:3983/64 dev dummy1
(can wait a few seconds if you want to, the above delete isn't [directly] the problem)
  # ip -6 addr show dev dummy1
11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
inet6 2000::1/64 scope global mngtmpaddr
valid_lft forever preferred_lft forever
inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
valid_lft forever preferred_lft forever
  # ip -6 addr del 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1
11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
      inet6 2000::81c9:56b7:f51a:b98f/64 scope global temporary dynamic
         valid_lft 604797sec preferred_lft 86169sec
      inet6 fe80::e8a8:a6ff:fed5:56d4/64 scope link
         valid_lft forever preferred_lft forever
This patch prevents this new 'global temporary dynamic' address from being created by the deletion of the related (same subnet prefix) 'mngtmpaddr' (which is triggered by there already being no temporary addresses).
Cc "David S. Miller" <[email protected]>
Cc David Ahern <[email protected]>
Cc Jiri Pirko <[email protected]>
Fixes 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")
Reported-by Xiao Ma <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>

Quoted 4 lines

---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Reviewed-by David Ahern <[email protected]>

[email protected]

CI Applied

2023-07-24 23:00 UTC #17 lore raw

In reply to Maciej Żenczykowski

This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Thu, 20 Jul 2023 09:00:22 -0700 you wrote:

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

[...]

Here is the summary with links:
- [net,v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

    https://git.kernel.org/netdev/net/c/69172f0bcb6a

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Maciej Żenczykowski

2023-07-25 04:59 UTC #18 lore raw

Quoted 5 lines · On Tue, Jul 25, 2023 at 1:00 AM <[email protected]> wrote:

Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Thank you.

Quoted 11 lines · On Thu, 20 Jul 2023 09:00:22 -0700 you wrote:

Quoted 9 lines

currently on 6.4 net/main:

  # ip link add dummy1 type dummy
  # echo 1 > /proc/sys/net/ipv6/conf/dummy1/use_tempaddr
  # ip link set dummy1 up
  # ip -6 addr add 2000::1/64 mngtmpaddr dev dummy1
  # ip -6 addr show dev dummy1

[...]

Quoted 10 lines

Here is the summary with links:
- [net,v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address
    https://git.kernel.org/netdev/net/c/69172f0bcb6a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Maciej Żenczykowski, Kernel Networking Developer @ Google