From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode Date: Fri, 6 Jul 2018 17:02:14 +0200 Message-ID: <20180706150214.GA25283@bistromath.localdomain> References: <3c53aeaf-912f-8921-c9df-40f2caa1747a@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: netdev@vger.kernel.org, Jiri Pirko , Felix Jia To: David Ahern Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:57692 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932769AbeGFPCJ (ORCPT ); Fri, 6 Jul 2018 11:02:09 -0400 Content-Disposition: inline In-Reply-To: <3c53aeaf-912f-8921-c9df-40f2caa1747a@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: 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