Re: [PATCH] netns: remove useless synchronize_net()

[Nicolas Dichtel <nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

Daniel Lezcano wrote:
> Daniel Lezcano wrote:
>> Eric W. Biederman wrote:
>>  
>>> Daniel Lezcano <daniel.lezcano-GANU6spQydw@public.gmane.org> writes:
>>>
>>>      
>>>> Eric W. Biederman wrote:
>>>>          
>>>>> Daniel Lezcano <daniel.lezcano-GANU6spQydw@public.gmane.org> writes:
>>>>>                
>>>>>> Hmm, at the first glance I would say it is useless but perhaps 
>>>>>> there is a
>>>>>>                   
>>>> trick
>>>>          
>>>>>> here I do not understand.
>>>>>> Eric, is there any particular reason to call synchronize_net 
>>>>>> before exiting
>>>>>>                   
>>>> the
>>>>          
>>>>>> dev_change_net_namespace function ?
>>>>>>                       
>>>>> I haven't thought about that part of the code path in detail in a long
>>>>> time.  dev_change_net_namespace() is a condensed version of
>>>>> register_netdevice() unregister_netdevice().  With the calls down into
>>>>> the driver removed.
>>>>>
>>>>> On a side note.  It looks like we now cope with:
>>>>> call_netdevice_notifiers(NETDEV_REGISTER, dev); failing in
>>>>> register_netdev, but no one updated dev_change_net_namespace to handle
>>>>> the change, looks like a real pain to cope with.
>>>>>
>>>>> As for the synchronize_net, and in response to the original
>>>>> comment as best as I can tell we do have things being being
>>>>> deleted that are at least candidates for synchronize_net.
>>>>>
>>>>> dev_addr_discard(dev);
>>>>> dev_net_set(dev, net);
>>>>> netdev_unregister_kobject(dev);
>>>>>
>>>>> We very much do access dev->net with only rcu protection.
>>>>>
>>>>> Hmm.
>>>>>
>>>>> It looks like I originally took the second synchronize_net from what
>>>>> became rollback_registered, which happens just before we start freeing
>>>>> the netdevice.
>>>>>
>>>>> The nastiest case that I can envision is if we happen to receive a
>>>>> packet (on another cpu) for the network device that we are moving,
>>>>> just after it has registered in the new network namespace.  If we read
>>>>> the old network namespace and forward it up the network stack in that
>>>>> context I can imagine it being a recipe for all kinds of strange
>>>>> non-deterministic behavior.
>>>>>                 
>>>> The code does:
>>>>
>>>>    dev_close
>>>>       dev_deactive
>>>>          synchronize_rcu
>>>>    synchronize_net
>>>>    ...
>>>>    dev_shutdown
>>>>    ...
>>>>    synchronize_net
>>>>
>>>> The network device can no longer receive packets after dev_deactive, 
>>>> no ?
>>>> The first synchronize_net will wait for the outstanding packets to 
>>>> be delivered
>>>> to the upper layer and we change the nd_net field after.
>>>> Your scenario makes sense for the first synchronize_net but I am not 
>>>> sure that
>>>> can happen if we remove the second synchronize_net.
>>>>           
>>> Good point.  Visibility is key.  What can find us after we
>>> call list_netdevice() ?  Aren't there some pieces of code that
>>> do for_each_netdevice under the rcu lock?
>>>       
>> AFAIR, no. for_each_netdev is protected by rtnl_lock.
>>   
> 
> Nicolas,
> 
> At the first glance it looks like the removing of the second 
> synchronize_net is fine, but before posting the patch do you mind to 
> wait a little ?
> I would like to do some tests with your patch to check if we don't 
> missed something.
> 

Hi Daniel,

no problem, there is no hurry. Let me know the result of your tests.

Thanks,
Nicolas