[PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[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" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Jiri Pirko <jiri@resnulli.us>
Fixes: 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e5213e598a04..94cec2075eee 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -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);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Wed, Jul 12, 2023 at 3:55 PM Maciej Żenczykowski <maze@google.com> 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
>
>   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" <davem@davemloft.net>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Fixes: 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  net/ipv6/addrconf.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index e5213e598a04..94cec2075eee 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -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);
> --
> 2.41.0.255.g8b1d071c50-goog
>

eh, should have included:

Reported-by: Xiao Ma <xiaom@google.com>

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[David Ahern]

On 7/12/23 7:55 AM, Maciej Żenczykowski wrote:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index e5213e598a04..94cec2075eee 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -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;

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`.


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

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Thu, Jul 13, 2023 at 4:59 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 7/12/23 7:55 AM, Maciej Żenczykowski wrote:
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index e5213e598a04..94cec2075eee 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -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;
>
> 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...

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[David Ahern]

On 7/13/23 9:03 AM, Maciej Żenczykowski wrote:
> On Thu, Jul 13, 2023 at 4:59 PM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 7/12/23 7:55 AM, Maciej Żenczykowski wrote:
>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>> index e5213e598a04..94cec2075eee 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -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;
>>
>> 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...

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.

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Jakub Kicinski]

On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
> > 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?

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[David Ahern]

On 7/18/23 5:08 PM, Jakub Kicinski wrote:
> On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
>>> 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?

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.

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Jiri Pirko]

Wed, Jul 19, 2023 at 01:08:32AM CEST, kuba@kernel.org wrote:
>On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
>> > 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?

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 :/

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Wed, Jul 19, 2023 at 8:47 AM Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Jul 19, 2023 at 01:08:32AM CEST, kuba@kernel.org wrote:
> >On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
> >> > 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?
>
> 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...

Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[David Ahern]

On 7/19/23 6:50 AM, Maciej Żenczykowski wrote:
> On Wed, Jul 19, 2023 at 8:47 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Jul 19, 2023 at 01:08:32AM CEST, kuba@kernel.org wrote:
>>> On Fri, 14 Jul 2023 08:49:55 -0600 David Ahern wrote:
>>>>> 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?
>>
>> 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...

ok, want to resend this patch?

[PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[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" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Jiri Pirko <jiri@resnulli.us>
Fixes: 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")
Reported-by: Xiao Ma <xiaom@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/ipv6/addrconf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e5213e598a04..94cec2075eee 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -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);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Jakub Kicinski]

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.

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Thu, Jul 20, 2023 at 9:34 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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.

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...)

> 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.

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

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

On Thu, Jul 20, 2023 at 6:52 PM Maciej Żenczykowski <maze@google.com> wrote:
>
> On Thu, Jul 20, 2023 at 9:34 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > 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.
>
> 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...)
>
> > 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

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Jakub Kicinski]

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

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Mon, Jul 24, 2023 at 8:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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).

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

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

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

Thanks!

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[David Ahern]

On 7/20/23 10:00 AM, 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
> 
>   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" <davem@davemloft.net>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Fixes: 53bd67491537 ("ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses")
> Reported-by: Xiao Ma <xiaom@google.com>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  net/ipv6/addrconf.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

[Maciej Żenczykowski]

On Tue, Jul 25, 2023 at 1:00 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to netdev/net.git (main)
> by Jakub Kicinski <kuba@kernel.org>:

Thank you.

> 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!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>
Maciej Żenczykowski, Kernel Networking Developer @ Google