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

[Sabrina Dubroca <sd@queasysnail.net>]

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