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. Thanks -- Daniel _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers