From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH net 1/3] net/ipv6: fix addrconf_sysctl_addr_gen_mode
Date: Fri, 6 Jul 2018 09:28:48 -0600
Message-ID: <99f4b357-741a-e6e4-1ced-bf70aa9a0262@gmail.com>
References: <cover.1530884498.git.sd@queasysnail.net>
 <f5e0fd3bdfdb6a11e13879c209b0fe5364f136f2.1530884498.git.sd@queasysnail.net>
 <3c53aeaf-912f-8921-c9df-40f2caa1747a@gmail.com>
 <20180706150214.GA25283@bistromath.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, Jiri Pirko <jiri@resnulli.us>,
        Felix Jia <felix.jia@alliedtelesis.co.nz>
To: Sabrina Dubroca <sd@queasysnail.net>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf0-f196.google.com ([209.85.192.196]:43467 "EHLO
        mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753814AbeGFP2Q (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 6 Jul 2018 11:28:16 -0400
Received: by mail-pf0-f196.google.com with SMTP id y8-v6so8722637pfm.10
        for <netdev@vger.kernel.org>; Fri, 06 Jul 2018 08:28:15 -0700 (PDT)
In-Reply-To: <20180706150214.GA25283@bistromath.localdomain>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.