netdev - Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcf463b0-9d1c-e248-07d8-6be5ebd96ecd@gmail.com>
Date:   Tue, 10 Jul 2018 06:19:04 -0600
From:   David Ahern <dsahern@...il.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Felix Jia <felix.jia@...iedtelesis.co.nz>
Subject: Re: [PATCH net v2 4/5] net/ipv6: propagate
 net.ipv6.conf.all.addr_gen_mode to devices

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 <dsahern@...il.com>
>>> Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
>>> ---
>>>  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: <BROADCAST,MULTICAST,UP,LOWER_UP> 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: <BROADCAST,MULTICAST,UP,LOWER_UP> 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.