| Message ID | 20230712135520.743211-1-maze@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address | expand |
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>
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);
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...
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.
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?
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.
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 :/
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...
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?
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);
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(-)