Re: [PATCH net-next] ipv6: recreate ipv6 link-local addresses when increasing MTU over IPV6_MIN_MTU

[Alexander Duyck <alexander.duyck@gmail.com>]

On 10/26/2015 09:05 AM, Hannes Frederic Sowa wrote:
> Hi Alex,
>
> On Mon, Oct 26, 2015, at 16:52, Alexander Duyck wrote:
>> On 10/26/2015 07:36 AM, Hannes Frederic Sowa wrote:
>>> Take into consideration that the interface might be disabled for IPv6,
>>> thus switch event type.
>>>
>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>> ---
>>>    net/ipv6/addrconf.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>> index d0c685c..c2dcebe 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -3149,6 +3149,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>>>    
>>>    	case NETDEV_UP:
>>>    	case NETDEV_CHANGE:
>>> +netdev_change:
>>>    		if (dev->flags & IFF_SLAVE)
>>>    			break;
>>>    
>>> @@ -3244,8 +3245,10 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>>>    
>>>    		if (!idev && dev->mtu >= IPV6_MIN_MTU) {
>>>    			idev = ipv6_add_dev(dev);
>>> -			if (!IS_ERR(idev))
>>> -				break;
>>> +			if (!IS_ERR(idev)) {
>>> +				event = NETDEV_UP;
>>> +				goto netdev_change;
>>> +			}
>>>    		}
>>>    
>>>    		/*
>> Seems like this code isn't quite correct.  You are calling ipv6_add_dev
>> for slave devices, and if I understand things correctly I don't believe
>> that was happening before and may be an unintended side effect.
> Hmm, could you quickly help me where I get into this situation? I made
> sure I enter the NETDEV_UP part before the IFF_SLAVE test and
> disable_ipv6 te

I think I was getting a bit a head of myself.  I was looking over the 
NETDEV_UP code and thinking that we could just fall into that path since 
it is already calling ipv6_add_dev.  However now I am wondering if maybe 
we need to look at adding an idev allocation somewhere before the 
disable_ipv6 check.  I assume that is why you were allocating the idev 
before you were getting into NETDEV_UP?

>> You might want to instead just make it so that you only do the jump, and
>> perhaps change the code in the NETDEV_UP/NETDEV_CHANGE section so that
>> you test for NETDEV_CHANGE instead of NETDEV_UP.  That should be enough
>> to get the effect you are looking for and I believe there would be no
>> change to behaviour other than adding IPv6 link-local addresses when the
>> MTU is increased.
>>
>> Give me a bit and I can submit an alternative that may actually work out
>> a bit better I think.
> If you go the NETDEV_CHANGE route instead of NETDEV_UP, you end up with
> the IF_READY flag already set from ipv6_add_dev and thus won't do any
> initialization of the device.

What I meant was that you don't need to change the event.  If you change 
the check inside the NETDEV_UP/CHANGE code path so that it tests for 
event != NETDEV_CHANGE instead of event == NETDEV_UP you don't need to 
change the event type.

> Sure, I wait.

Might be a bit longer.  I just realized that I think there is another 
bug here where you are going through the NETDEV_UP path even though the 
interface isn't up.  I'll run through some testing this morning to work 
out the kinks.

- Alex