[PATCH net 0/3] net/ipv6: addr_gen_mode fixes

[PATCH net 0/3] 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.

Sabrina Dubroca (3):
  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/addrconf.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

-- 
2.18.0

[PATCH net 1/3] 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

[PATCH net 2/3] 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>
---
 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 3/3] 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>
---
 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

Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[David Ahern]

On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
> 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;

updating the template is wrong, but you still need to update the
namespace's default value for new devices.

also, if you are fixing this it would be good to handle the change to
'all' as well and update all existing devices.

> -
> -		/* 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:
> 

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

[David Ahern]

On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
> 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>
> ---
>  net/ipv6/addrconf.c | 2 --
>  1 file changed, 2 deletions(-)
> 

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

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

[David Ahern]

On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
> 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>
> ---
>  net/ipv6/addrconf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

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

Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[Sabrina Dubroca]

2018-07-06, 08:42:01 -0600, David Ahern wrote:
> On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
> > 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;
> 
> updating the template is wrong, but you still need to update the
> namespace's default value for new devices.

That's already handled by storing new_val into ctl->data at the end of
the 'write' block.

BTW, I'd like to make ipv6_devconf and ipv6_devconf_dflt read-only, so
that people aren't tempted to update the template, but I'm thinking of
doing that in net-next rather than net.

> also, if you are fixing this it would be good to handle the change to
> 'all' as well and update all existing devices.

I thought about it, and wasn't sure if that change of behavior was
acceptable, especially for stable (I think the current patch should go
into stable). OTOH, it's quite clearly what "all" should do.

> > -
> > -		/* 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:
> > 
> 

-- 
Sabrina

Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[David Ahern]

On 7/6/18 9:02 AM, Sabrina Dubroca wrote:
> 2018-07-06, 08:42:01 -0600, David Ahern wrote:
>> On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
>>> 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;
>>
>> updating the template is wrong, but you still need to update the
>> namespace's default value for new devices.
> 
> That's already handled by storing new_val into ctl->data at the end of
> the 'write' block.

ok. missed that. It's part of your change below.

> 
> BTW, I'd like to make ipv6_devconf and ipv6_devconf_dflt read-only, so
> that people aren't tempted to update the template, but I'm thinking of
> doing that in net-next rather than net.
> 
>> also, if you are fixing this it would be good to handle the change to
>> 'all' as well and update all existing devices.
> 
> I thought about it, and wasn't sure if that change of behavior was
> acceptable, especially for stable (I think the current patch should go
> into stable). OTOH, it's quite clearly what "all" should do.

IMHO it's a bug that changing 'all' does not actually change all
existing devices nor is it ever used.

Looking at other addr_gen_mode sites, addrconf_sysctl_stable_secret is
messed up as well. It propagates a change to 'default' to all existing
devices.

Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[Sabrina Dubroca]

2018-07-06, 09:28:48 -0600, David Ahern wrote:
> On 7/6/18 9:02 AM, Sabrina Dubroca wrote:
> > 2018-07-06, 08:42:01 -0600, David Ahern wrote:
> >> On 7/6/18 7:49 AM, Sabrina Dubroca wrote:
> >>> 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;
> >>
> >> updating the template is wrong, but you still need to update the
> >> namespace's default value for new devices.
> > 
> > That's already handled by storing new_val into ctl->data at the end of
> > the 'write' block.
> 
> ok. missed that. It's part of your change below.
> 
> > 
> > BTW, I'd like to make ipv6_devconf and ipv6_devconf_dflt read-only, so
> > that people aren't tempted to update the template, but I'm thinking of
> > doing that in net-next rather than net.
> > 
> >> also, if you are fixing this it would be good to handle the change to
> >> 'all' as well and update all existing devices.
> > 
> > I thought about it, and wasn't sure if that change of behavior was
> > acceptable, especially for stable (I think the current patch should go
> > into stable). OTOH, it's quite clearly what "all" should do.
> 
> IMHO it's a bug that changing 'all' does not actually change all
> existing devices nor is it ever used.

Right. I'll add that as a separate patch in this series, unless you
really prefer the change squashed into this patch.


> Looking at other addr_gen_mode sites, addrconf_sysctl_stable_secret is
> messed up as well. It propagates a change to 'default' to all existing
> devices.

I guess it was intentional, given:

        if (&net->ipv6.devconf_all->stable_secret == ctl->data)
                return -EIO;

It only propagates the mode, and not the secret itself, to all
devices. After thinking about it for a while, I guess it considers the
new default not only as default for newly created devices, but also
for newly added addresses/prefixes.
Or am I making stuff up?

-- 
Sabrina

Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode

[David Ahern]

On 7/6/18 9:58 AM, Sabrina Dubroca wrote:
> 
> Right. I'll add that as a separate patch in this series, unless you
> really prefer the change squashed into this patch.

no preference.

> 
> 
>> Looking at other addr_gen_mode sites, addrconf_sysctl_stable_secret is
>> messed up as well. It propagates a change to 'default' to all existing
>> devices.
> 
> I guess it was intentional, given:
> 
>         if (&net->ipv6.devconf_all->stable_secret == ctl->data)
>                 return -EIO;
> 
> It only propagates the mode, and not the secret itself, to all
> devices. After thinking about it for a while, I guess it considers the
> new default not only as default for newly created devices, but also
> for newly added addresses/prefixes.
> Or am I making stuff up?
> 

Maybe Hannes can explain (622c81d57b392). It should have been all
instead of default. As I understand it default is what devices start
with on create and individual devices can be changed. 'All' overrides
device-specific settings. So the above is inconsistent. One of many with
sysctl that makes it frustrating for users.