* [PATCH] net namespace: wrap for_each_net with rtnl_lock @ 2013-07-04 6:20 Fan Du 2013-07-04 7:04 ` Eric W. Biederman 0 siblings, 1 reply; 3+ messages in thread From: Fan Du @ 2013-07-04 6:20 UTC (permalink / raw) To: ebiederm; +Cc: serge.hallyn, davem, netdev The read access to net_namespace_list with for_each_net should always be protected with rtnl_lock agiast any adding/removing operation from the list. Signed-off-by: Fan Du <fan.du@windriver•com> --- net/core/net_namespace.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c index f976520..f3808ff 100644 --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -445,12 +445,14 @@ static int __register_pernet_operations(struct list_head *list, list_add_tail(&ops->list, list); if (ops->init || (ops->id && ops->size)) { + rtnl_lock(); for_each_net(net) { error = ops_init(ops, net); if (error) goto out_undo; list_add_tail(&net->exit_list, &net_exit_list); } + rtnl_unlock(); } return 0; @@ -468,8 +470,10 @@ static void __unregister_pernet_operations(struct pernet_operations *ops) LIST_HEAD(net_exit_list); list_del(&ops->list); + rtnl_lock(); for_each_net(net) list_add_tail(&net->exit_list, &net_exit_list); + rtnl_unlock(); ops_exit_list(ops, &net_exit_list); ops_free_list(ops, &net_exit_list); } -- 1.7.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net namespace: wrap for_each_net with rtnl_lock 2013-07-04 6:20 [PATCH] net namespace: wrap for_each_net with rtnl_lock Fan Du @ 2013-07-04 7:04 ` Eric W. Biederman 2013-07-04 7:57 ` Fan Du 0 siblings, 1 reply; 3+ messages in thread From: Eric W. Biederman @ 2013-07-04 7:04 UTC (permalink / raw) To: Fan Du; +Cc: serge.hallyn, davem, netdev Fan Du <fan.du@windriver•com> writes: > The read access to net_namespace_list with for_each_net should always > be protected with rtnl_lock agiast any adding/removing operation from > the list. That is not correct. The rtnl_lock does not protect the net_namespace_list. The net_mutex provides that protection. Modifications to the net_namespace_list are under both the rtnl_lock and the net_mutex which removes the need of grabbing the net_mutex when you just need to traverse the list of network namespaces. This avoids a lock ordering problem as most places it is desirable to traverse the net namespace list the rtnl_lock is already held. In general the init methods will deadlock if you call them with the rtnl_lock held, as they grab the rtnl_lock when creating network devices etc. The methods you change are protected by the net_mutex so I don't see any problems here. Was this patch inspired by code review or was there an actual problem that inspired it? > Signed-off-by: Fan Du <fan.du@windriver•com> > --- > net/core/net_namespace.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c > index f976520..f3808ff 100644 > --- a/net/core/net_namespace.c > +++ b/net/core/net_namespace.c > @@ -445,12 +445,14 @@ static int __register_pernet_operations(struct list_head *list, > > list_add_tail(&ops->list, list); > if (ops->init || (ops->id && ops->size)) { > + rtnl_lock(); > for_each_net(net) { > error = ops_init(ops, net); > if (error) > goto out_undo; > list_add_tail(&net->exit_list, &net_exit_list); > } > + rtnl_unlock(); > } > return 0; > > @@ -468,8 +470,10 @@ static void __unregister_pernet_operations(struct pernet_operations *ops) > LIST_HEAD(net_exit_list); > > list_del(&ops->list); > + rtnl_lock(); > for_each_net(net) > list_add_tail(&net->exit_list, &net_exit_list); > + rtnl_unlock(); > ops_exit_list(ops, &net_exit_list); > ops_free_list(ops, &net_exit_list); > } ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net namespace: wrap for_each_net with rtnl_lock 2013-07-04 7:04 ` Eric W. Biederman @ 2013-07-04 7:57 ` Fan Du 0 siblings, 0 replies; 3+ messages in thread From: Fan Du @ 2013-07-04 7:57 UTC (permalink / raw) To: Eric W. Biederman; +Cc: serge.hallyn, davem, netdev Hi, Eric Thanks for your reply! On 2013年07月04日 15:04, Eric W. Biederman wrote: > Fan Du<fan.du@windriver•com> writes: > >> The read access to net_namespace_list with for_each_net should always >> be protected with rtnl_lock agiast any adding/removing operation from >> the list. > > That is not correct. The rtnl_lock does not protect the > net_namespace_list. The net_mutex provides that protection. > > Modifications to the net_namespace_list are under both the rtnl_lock > and the net_mutex which removes the need of grabbing the net_mutex when > you just need to traverse the list of network namespaces. This avoids a > lock ordering problem as most places it is desirable to traverse the > net namespace list the rtnl_lock is already held. By my understanding, net_mutex protects operations on pernet_list, and rtln_lock protects net_namespace_list. net_mutex has side effects on net_namespace_list, because we try to hold rtnl_lock to modify net_namespace_list after already holding net_mutex(copy_net_ns). Sorry, I cann't understand the necessity by doing so. (1) mutex_lock(&net_mutex); rv = setup_net(net, user_ns); if (rv == 0) { rtnl_lock(); list_add_tail_rcu(&net->list, &net_namespace_list); rtnl_unlock(); } mutex_unlock(&net_mutex); why could we do it separately as below? (2) mutex_lock(&net_mutex); rv = setup_net(net, user_ns); mutex_unlock(&net_mutex); if (rv == 0) { rtnl_lock(); list_add_tail_rcu(&net->list, &net_namespace_list); rtnl_unlock(); } > > In general the init methods will deadlock if you call them with the > rtnl_lock held, as they grab the rtnl_lock when creating network devices > etc. Yes, I understand. This is reason why we do it in (1) style. > The methods you change are protected by the net_mutex so I don't see any > problems here. > Was this patch inspired by code review or was there an actual problem > that inspired it? It's code review, no real alarm :) >> Signed-off-by: Fan Du<fan.du@windriver•com> >> --- >> net/core/net_namespace.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c >> index f976520..f3808ff 100644 >> --- a/net/core/net_namespace.c >> +++ b/net/core/net_namespace.c >> @@ -445,12 +445,14 @@ static int __register_pernet_operations(struct list_head *list, >> >> list_add_tail(&ops->list, list); >> if (ops->init || (ops->id&& ops->size)) { >> + rtnl_lock(); >> for_each_net(net) { >> error = ops_init(ops, net); >> if (error) >> goto out_undo; >> list_add_tail(&net->exit_list,&net_exit_list); >> } >> + rtnl_unlock(); >> } >> return 0; >> >> @@ -468,8 +470,10 @@ static void __unregister_pernet_operations(struct pernet_operations *ops) >> LIST_HEAD(net_exit_list); >> >> list_del(&ops->list); >> + rtnl_lock(); >> for_each_net(net) >> list_add_tail(&net->exit_list,&net_exit_list); >> + rtnl_unlock(); >> ops_exit_list(ops,&net_exit_list); >> ops_free_list(ops,&net_exit_list); >> } > -- 浮沉随浪只记今朝笑 --fan ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-07-04 7:57 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-04 6:20 [PATCH] net namespace: wrap for_each_net with rtnl_lock Fan Du 2013-07-04 7:04 ` Eric W. Biederman 2013-07-04 7:57 ` Fan Du
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox