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.