Daniel Lezcano wrote: > Daniel Lezcano wrote: >> Eric W. Biederman wrote: >> >>> Daniel Lezcano <daniel.lezcano@xxxxxxx> writes: >>> >>> >>>> Eric W. Biederman wrote: >>>> >>>>> Daniel Lezcano <daniel.lezcano@xxxxxxx> 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 _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers