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: <20180710101352.GA31570@bistromath.localdomain>
Date:   Tue, 10 Jul 2018 12:13:52 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     David Ahern <dsahern@...il.com>
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

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.

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.

-- 
Sabrina