[PATCH net v2 0/5] net/ipv6: addr_gen_mode fixes

[PATCH net v2 0/5] net/ipv6: addr_gen_mode fixes

[Sabrina Dubroca]

This series fixes bugs in handling of the addr_gen_mode option, mainly
related to the sysctl. A minor netlink issue was also present in the
initial commit introducing the option on a per-netdevice basis.

v2: add patch 4, requested by David Ahern during review of v1
    add patch 5, missing documentation for the sysctl
    patches 1, 2, 3 are unchanged

Sabrina Dubroca (5):
  net/ipv6: fix addrconf_sysctl_addr_gen_mode
  net/ipv6: don't reinitialize ndev->cnf.addr_gen_mode on new inet6_dev
  net/ipv6: reserve room for IFLA_INET6_ADDR_GEN_MODE
  net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode
  Documentation: ip-sysctl.txt: document addr_gen_mode

 Documentation/networking/ip-sysctl.txt | 10 ++++++++
 net/ipv6/addrconf.c                    | 45 ++++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.18.0

[PATCH net v2 1/5] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[Sabrina Dubroca]

addrconf_sysctl_addr_gen_mode() has multiple problems. First, it ignores
the errors returned by proc_dointvec().

addrconf_sysctl_addr_gen_mode() calls proc_dointvec() directly, which
writes the value to memory, and then checks if it's valid and may return
EINVAL. If a bad value is given, the value displayed when reading
net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the
value provided by the user was valid, addrconf_dev_config() won't be
called since idev->cnf.addr_gen_mode has already been updated.

Fix this in the usual way we deal with values that need to be checked
after the proc_do*() helper has returned: define a local ctl_table and
storage, call proc_dointvec() on that temporary area, then check and
store.

addrconf_sysctl_addr_gen_mode() also writes the new value to the global
ipv6_devconf_dflt, when we're writing to some netns's default, so that
new netns will inherit the value that was set by the change occuring in
any netns. That doesn't make any sense, so let's drop this assignment.

Finally, since addr_gen_mode is a __u32, switch to proc_douintvec().

Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/ipv6/addrconf.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 91580c62bb86..e9ba53d2a147 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5892,32 +5892,31 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write,
 					 loff_t *ppos)
 {
 	int ret = 0;
-	int new_val;
+	u32 new_val;
 	struct inet6_dev *idev = (struct inet6_dev *)ctl->extra1;
 	struct net *net = (struct net *)ctl->extra2;
+	struct ctl_table tmp = {
+		.data = &new_val,
+		.maxlen = sizeof(new_val),
+		.mode = ctl->mode,
+	};
 
 	if (!rtnl_trylock())
 		return restart_syscall();
 
-	ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
+	new_val = *((u32 *)ctl->data);
 
-	if (write) {
-		new_val = *((int *)ctl->data);
+	ret = proc_douintvec(&tmp, write, buffer, lenp, ppos);
+	if (ret != 0)
+		goto out;
 
+	if (write) {
 		if (check_addr_gen_mode(new_val) < 0) {
 			ret = -EINVAL;
 			goto out;
 		}
 
-		/* request for default */
-		if (&net->ipv6.devconf_dflt->addr_gen_mode == ctl->data) {
-			ipv6_devconf_dflt.addr_gen_mode = new_val;
-
-		/* request for individual net device */
-		} else {
-			if (!idev)
-				goto out;
-
+		if (idev) {
 			if (check_stable_privacy(idev, net, new_val) < 0) {
 				ret = -EINVAL;
 				goto out;
@@ -5928,6 +5927,8 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write,
 				addrconf_dev_config(idev->dev);
 			}
 		}
+
+		*((u32 *)ctl->data) = new_val;
 	}
 
 out:
-- 
2.18.0

Re: [PATCH net v2 1/5] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[David Ahern]

On 7/9/18 4:25 AM, Sabrina Dubroca wrote:
> addrconf_sysctl_addr_gen_mode() has multiple problems. First, it ignores
> the errors returned by proc_dointvec().
> 
> addrconf_sysctl_addr_gen_mode() calls proc_dointvec() directly, which
> writes the value to memory, and then checks if it's valid and may return
> EINVAL. If a bad value is given, the value displayed when reading
> net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the
> value provided by the user was valid, addrconf_dev_config() won't be
> called since idev->cnf.addr_gen_mode has already been updated.
> 
> Fix this in the usual way we deal with values that need to be checked
> after the proc_do*() helper has returned: define a local ctl_table and
> storage, call proc_dointvec() on that temporary area, then check and
> store.
> 
> addrconf_sysctl_addr_gen_mode() also writes the new value to the global
> ipv6_devconf_dflt, when we're writing to some netns's default, so that
> new netns will inherit the value that was set by the change occuring in
> any netns. That doesn't make any sense, so let's drop this assignment.
> 
> Finally, since addr_gen_mode is a __u32, switch to proc_douintvec().
> 
> Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  net/ipv6/addrconf.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

[PATCH net v2 2/5] net/ipv6: don't reinitialize ndev->cnf.addr_gen_mode on new inet6_dev

[Sabrina Dubroca]

The value has already been copied from this netns's devconf_dflt, it
shouldn't be reset to the global kernel default.

Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/addrconf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e9ba53d2a147..e20f8a1d8cdb 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -385,8 +385,6 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 
 	if (ndev->cnf.stable_secret.initialized)
 		ndev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
-	else
-		ndev->cnf.addr_gen_mode = ipv6_devconf_dflt.addr_gen_mode;
 
 	ndev->cnf.mtu6 = dev->mtu;
 	ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
-- 
2.18.0

[PATCH net v2 3/5] net/ipv6: reserve room for IFLA_INET6_ADDR_GEN_MODE

[Sabrina Dubroca]

inet6_ifla6_size() is called to check how much space is needed by
inet6_fill_link_af() and inet6_fill_ifinfo(), both of which include
the IFLA_INET6_ADDR_GEN_MODE attribute. Reserve some room for it.

Fixes: bc91b0f07ada ("ipv6: addrconf: implement address generation modes")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/addrconf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e20f8a1d8cdb..e89bca83e0e4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5208,7 +5208,9 @@ static inline size_t inet6_ifla6_size(void)
 	     + nla_total_size(DEVCONF_MAX * 4) /* IFLA_INET6_CONF */
 	     + nla_total_size(IPSTATS_MIB_MAX * 8) /* IFLA_INET6_STATS */
 	     + nla_total_size(ICMP6_MIB_MAX * 8) /* IFLA_INET6_ICMP6STATS */
-	     + nla_total_size(sizeof(struct in6_addr)); /* IFLA_INET6_TOKEN */
+	     + nla_total_size(sizeof(struct in6_addr)) /* IFLA_INET6_TOKEN */
+	     + nla_total_size(1) /* IFLA_INET6_ADDR_GEN_MODE */
+	     + 0;
 }
 
 static inline size_t inet6_if_nlmsg_size(void)
-- 
2.18.0

[PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices

[Sabrina Dubroca]

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@gmail.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.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);
+				}
+			}
 		}
 
 		*((u32 *)ctl->data) = new_val;
-- 
2.18.0

Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices

[David Ahern]

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@gmail.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.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


> +				}
> +			}
>  		}
>  
>  		*((u32 *)ctl->data) = new_val;
> 

Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices

[Sabrina Dubroca]

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@gmail.com>
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.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

Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices

[David Ahern]

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@gmail.com>
>>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.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.

[PATCH net v2 5/5] Documentation: ip-sysctl.txt: document addr_gen_mode

[Sabrina Dubroca]

addr_gen_mode was introduced in without documentation, add it now.

Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 Documentation/networking/ip-sysctl.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index ce8fbf5aa63c..c6df84be12f9 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1834,6 +1834,16 @@ stable_secret - IPv6 address
 
 	By default the stable secret is unset.
 
+addr_gen_mode - INTEGER
+	Defines how link-local and autoconf addresses are generated.
+
+	0: generate address based on EUI64 (default)
+	1: do no generate a link-local address, use EUI64 for addresses generated
+	   from autoconf
+	2: generate stable privacy addresses, using the secret from
+	   stable_secret (RFC7217)
+	3: generate stable privacy addresses, using a random secret if unset
+
 drop_unicast_in_l2_multicast - BOOLEAN
 	Drop any unicast IPv6 packets that are received in link-layer
 	multicast (or broadcast) frames.

Re: [PATCH net v2 5/5] Documentation: ip-sysctl.txt: document addr_gen_mode

[David Ahern]

On 7/9/18 4:25 AM, Sabrina Dubroca wrote:
> addr_gen_mode was introduced in without documentation, add it now.
> 
> Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  Documentation/networking/ip-sysctl.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net v2 0/5] net/ipv6: addr_gen_mode fixes

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Mon,  9 Jul 2018 12:25:13 +0200

> This series fixes bugs in handling of the addr_gen_mode option, mainly
> related to the sysctl. A minor netlink issue was also present in the
> initial commit introducing the option on a per-netdevice basis.
> 
> v2: add patch 4, requested by David Ahern during review of v1
>     add patch 5, missing documentation for the sysctl
>     patches 1, 2, 3 are unchanged

I know there is still some discussion going on about sysctl semantics,
but I'll aply this for now and any further refinements can be
submitted on top.

Thanks.