| Message ID | 20230720160022.1887942-1-maze@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 69172f0bcb6a09110c5d2a6d792627f5095a9018 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address | expand |
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.
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.
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
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
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!
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>
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!
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
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") 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(-)