From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices Date: Tue, 10 Jul 2018 06:19:04 -0600 Message-ID: References: <55a3cba92aefc90d3b1837e7fdf4b7e2fea05af4.1531129207.git.sd@queasysnail.net> <9e3ad74a-2321-9893-dbcb-5dec1d7ef054@gmail.com> <20180710101352.GA31570@bistromath.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Jiri Pirko , Felix Jia To: Sabrina Dubroca Return-path: Received: from mail-it0-f68.google.com ([209.85.214.68]:50929 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933216AbeGJMS2 (ORCPT ); Tue, 10 Jul 2018 08:18:28 -0400 Received: by mail-it0-f68.google.com with SMTP id w16-v6so8789477ita.0 for ; Tue, 10 Jul 2018 05:18:28 -0700 (PDT) In-Reply-To: <20180710101352.GA31570@bistromath.localdomain> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 7/10/18 4:13 AM, Sabrina Dubroca wrote: > 2018-07-09, 11:24:49 -0600, David Ahern wrote: >> On 7/9/18 4:25 AM, Sabrina Dubroca wrote: >>> This aligns the addr_gen_mode sysctl with the expected behavior of the >>> "all" variant. >>> >>> Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode") >>> Suggested-by: David Ahern >>> Signed-off-by: Sabrina Dubroca >>> --- >>> net/ipv6/addrconf.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >>> index e89bca83e0e4..1659a6b3cf42 100644 >>> --- a/net/ipv6/addrconf.c >>> +++ b/net/ipv6/addrconf.c >>> @@ -5926,6 +5926,18 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, >>> idev->cnf.addr_gen_mode = new_val; >>> addrconf_dev_config(idev->dev); >>> } >>> + } else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { >>> + struct net_device *dev; >>> + >>> + net->ipv6.devconf_dflt->addr_gen_mode = new_val; >>> + for_each_netdev(net, dev) { >>> + idev = __in6_dev_get(dev); >>> + if (idev && >>> + idev->cnf.addr_gen_mode != new_val) { >>> + idev->cnf.addr_gen_mode = new_val; >>> + addrconf_dev_config(idev->dev); >> >> This call is adding a new LL address without removing the previous one: >> >> # ip -6 addr sh dev eth2 >> 4: eth2: mtu 1500 state UP qlen 1000 >> inet6 2001:db8:2::4/64 scope global >> valid_lft forever preferred_lft forever >> inet6 fe80::e0:f9ff:fe45:6480/64 scope link >> valid_lft forever preferred_lft forever >> >> # sysctl -w net.ipv6.conf.eth2.addr_gen_mode=3 >> net.ipv6.conf.eth2.addr_gen_mode = 3 >> >> # ip -6 addr sh dev eth2 >> 4: eth2: mtu 1500 state UP qlen 1000 >> inet6 2001:db8:2::4/64 scope global >> valid_lft forever preferred_lft forever >> inet6 fe80::bc31:8009:270d:e019/64 scope link stable-privacy >> valid_lft forever preferred_lft forever >> inet6 fe80::e0:f9ff:fe45:6480/64 scope link >> valid_lft forever preferred_lft forever > > Yes. That's also what will happen with global addresses, once the next > RA is received: a new address corresponding to the new generation mode > will be added, and the old one isn't removed. Interesting. > > I think that was the expected behavior of d35a00b8e33d, but since it > never actually worked... OTOH, the netlink attribute only sets > idev->cnf.addr_gen_mode and doesn't add the new LL address (not until > a DOWN/UP cycle), which I personally find surprising. If I set the > mode to random or stable_secret, I would expect the privacy address to > show up without having to take the device down and then up. > > I think removing the previous address immediately would break things > (and the user wouldn't expect an address to disappear that way, since > they're not explicitly asking for it to be removed), but I guess we > could play games with the lifetimes (reduce the lifetime of the old > address from forever to some limit). That limit would need to be > configurable I think, and I would rather target that change for > net-next. > The setting is address generation mode. Addresses are generated on admin up only - I believe this is well established behavior. If the documentation contains a note that changing the mode requires an admin down/up, then it should not be a surprise to users. I can't imagine this setting is changed on the fly and expected to work immediately. Rather I would expect this is set at boot via the sysctl files or by an interface manager before interfaces are brought up. I'll ask around, but I am traveling today so will be a delay getting back on this.