* [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup @ 2015-03-25 19:05 Cong Wang 2015-03-25 19:05 ` [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list Cong Wang 2015-03-25 20:23 ` [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Hannes Frederic Sowa 0 siblings, 2 replies; 10+ messages in thread From: Cong Wang @ 2015-03-25 19:05 UTC (permalink / raw) To: netdev; +Cc: Cong Wang, Hannes Frederic Sowa This is the IPv4 part for commit 905a6f96a1b1 (ipv6: take rtnl_lock and mark mrt6 table as freed on namespace cleanup). Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/ipv4/ipmr.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c index 9d78427..d6fede8 100644 --- a/net/ipv4/ipmr.c +++ b/net/ipv4/ipmr.c @@ -278,10 +278,12 @@ static void __net_exit ipmr_rules_exit(struct net *net) { struct mr_table *mrt, *next; + rtnl_lock(); list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { list_del(&mrt->list); ipmr_free_table(mrt); } + rtnl_unlock(); fib_rules_unregister(net->ipv4.mr_rules_ops); } #else @@ -308,7 +310,10 @@ static int __net_init ipmr_rules_init(struct net *net) static void __net_exit ipmr_rules_exit(struct net *net) { + rtnl_lock(); ipmr_free_table(net->ipv4.mrt); + net->ipv4.mrt = NULL; + rtnl_unlock(); } #endif -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 19:05 [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Cong Wang @ 2015-03-25 19:05 ` Cong Wang 2015-03-25 20:07 ` Hannes Frederic Sowa 2015-03-25 20:23 ` [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Hannes Frederic Sowa 1 sibling, 1 reply; 10+ messages in thread From: Cong Wang @ 2015-03-25 19:05 UTC (permalink / raw) To: netdev; +Cc: Cong Wang Probably not a big deal, just for corretness. Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/ipv4/ipmr.c | 2 +- net/ipv6/ip6mr.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c index d6fede8..68f67b8 100644 --- a/net/ipv4/ipmr.c +++ b/net/ipv4/ipmr.c @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net *net) rtnl_lock(); list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { - list_del(&mrt->list); + list_del_rcu(&mrt->list); ipmr_free_table(mrt); } rtnl_unlock(); diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index 34b6826..3df1ec2 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -264,7 +264,7 @@ static void __net_exit ip6mr_rules_exit(struct net *net) rtnl_lock(); list_for_each_entry_safe(mrt, next, &net->ipv6.mr6_tables, list) { - list_del(&mrt->list); + list_del_rcu(&mrt->list); ip6mr_free_table(mrt); } rtnl_unlock(); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 19:05 ` [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list Cong Wang @ 2015-03-25 20:07 ` Hannes Frederic Sowa 2015-03-25 20:21 ` Hannes Frederic Sowa 0 siblings, 1 reply; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-25 20:07 UTC (permalink / raw) To: Cong Wang, netdev On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > Probably not a big deal, just for corretness. > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/ipv4/ipmr.c | 2 +- > net/ipv6/ip6mr.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > index d6fede8..68f67b8 100644 > --- a/net/ipv4/ipmr.c > +++ b/net/ipv4/ipmr.c > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net > *net) > > rtnl_lock(); > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { > - list_del(&mrt->list); > + list_del_rcu(&mrt->list); > ipmr_free_table(mrt); > } > rtnl_unlock(); I really do wonder if we have the rcu locking correct in there: Looking into getsockopt/setsockopt operations, we might have socket lock, but I cannot see where we lock rcu, so the ipmr_get_table call is safe. Do you also see the problem? Thanks, Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 20:07 ` Hannes Frederic Sowa @ 2015-03-25 20:21 ` Hannes Frederic Sowa 2015-03-25 20:56 ` Hannes Frederic Sowa 0 siblings, 1 reply; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-25 20:21 UTC (permalink / raw) To: Cong Wang, netdev On Wed, Mar 25, 2015, at 21:07, Hannes Frederic Sowa wrote: > On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > > Probably not a big deal, just for corretness. > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > --- > > net/ipv4/ipmr.c | 2 +- > > net/ipv6/ip6mr.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > > index d6fede8..68f67b8 100644 > > --- a/net/ipv4/ipmr.c > > +++ b/net/ipv4/ipmr.c > > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net > > *net) > > > > rtnl_lock(); > > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { > > - list_del(&mrt->list); > > + list_del_rcu(&mrt->list); > > ipmr_free_table(mrt); > > } > > rtnl_unlock(); > > I really do wonder if we have the rcu locking correct in there: > > Looking into getsockopt/setsockopt operations, we might have socket > lock, but I cannot see where we lock rcu, so the ipmr_get_table call is > safe. Do you also see the problem? Also ipmr_free_table does need a kfree_rcu as we need to have those tables rcu protected (we use them from softirq, so rtnl_lock is not feasible here). Bye, Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 20:21 ` Hannes Frederic Sowa @ 2015-03-25 20:56 ` Hannes Frederic Sowa 2015-03-25 22:33 ` Cong Wang 0 siblings, 1 reply; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-25 20:56 UTC (permalink / raw) To: Cong Wang, netdev On Wed, Mar 25, 2015, at 21:21, Hannes Frederic Sowa wrote: > On Wed, Mar 25, 2015, at 21:07, Hannes Frederic Sowa wrote: > > On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > > > Probably not a big deal, just for corretness. > > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > --- > > > net/ipv4/ipmr.c | 2 +- > > > net/ipv6/ip6mr.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > > > index d6fede8..68f67b8 100644 > > > --- a/net/ipv4/ipmr.c > > > +++ b/net/ipv4/ipmr.c > > > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net > > > *net) > > > > > > rtnl_lock(); > > > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { > > > - list_del(&mrt->list); > > > + list_del_rcu(&mrt->list); > > > ipmr_free_table(mrt); > > > } > > > rtnl_unlock(); > > > > I really do wonder if we have the rcu locking correct in there: > > > > Looking into getsockopt/setsockopt operations, we might have socket > > lock, but I cannot see where we lock rcu, so the ipmr_get_table call is > > safe. Do you also see the problem? > > Also ipmr_free_table does need a kfree_rcu as we need to have those > tables rcu protected (we use them from softirq, so rtnl_lock is not > feasible here). So, ipmr_free_tables is only called from within netns cleanup, which is run after synchronize_rcu(), so the kfree is safe. We only add tables to the list, no tables are ever deleted until the namespace gets destroyed, so the locking seems ok to me, but please double check. Thus I agree with your conclusion it should not really matter, so Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> Thanks, Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 20:56 ` Hannes Frederic Sowa @ 2015-03-25 22:33 ` Cong Wang 2015-03-26 1:01 ` Hannes Frederic Sowa 0 siblings, 1 reply; 10+ messages in thread From: Cong Wang @ 2015-03-25 22:33 UTC (permalink / raw) To: Hannes Frederic Sowa; +Cc: Cong Wang, netdev On Wed, Mar 25, 2015 at 1:56 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Wed, Mar 25, 2015, at 21:21, Hannes Frederic Sowa wrote: >> On Wed, Mar 25, 2015, at 21:07, Hannes Frederic Sowa wrote: >> > On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: >> > > Probably not a big deal, just for corretness. >> > > >> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> > > --- >> > > net/ipv4/ipmr.c | 2 +- >> > > net/ipv6/ip6mr.c | 2 +- >> > > 2 files changed, 2 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c >> > > index d6fede8..68f67b8 100644 >> > > --- a/net/ipv4/ipmr.c >> > > +++ b/net/ipv4/ipmr.c >> > > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net >> > > *net) >> > > >> > > rtnl_lock(); >> > > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { >> > > - list_del(&mrt->list); >> > > + list_del_rcu(&mrt->list); >> > > ipmr_free_table(mrt); >> > > } >> > > rtnl_unlock(); >> > >> > I really do wonder if we have the rcu locking correct in there: >> > >> > Looking into getsockopt/setsockopt operations, we might have socket >> > lock, but I cannot see where we lock rcu, so the ipmr_get_table call is >> > safe. Do you also see the problem? I see only ipmr_rule_action() really has RCU read lock, I think the sockopt operations should take RCU read lock too, since it is supposed to be protected by rcu+rtnl rather than rcu+sock lock? >> >> Also ipmr_free_table does need a kfree_rcu as we need to have those >> tables rcu protected (we use them from softirq, so rtnl_lock is not >> feasible here). > > So, ipmr_free_tables is only called from within netns cleanup, which is > run after synchronize_rcu(), so the kfree is safe. We only add tables to > the list, no tables are ever deleted until the namespace gets destroyed, > so the locking seems ok to me, but please double check. > Right, I found this by code review, didn't see any real crash. Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-25 22:33 ` Cong Wang @ 2015-03-26 1:01 ` Hannes Frederic Sowa 2015-03-26 12:23 ` Hannes Frederic Sowa 0 siblings, 1 reply; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-26 1:01 UTC (permalink / raw) To: Cong Wang; +Cc: Cong Wang, netdev On Wed, Mar 25, 2015, at 23:33, Cong Wang wrote: > On Wed, Mar 25, 2015 at 1:56 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > On Wed, Mar 25, 2015, at 21:21, Hannes Frederic Sowa wrote: > >> On Wed, Mar 25, 2015, at 21:07, Hannes Frederic Sowa wrote: > >> > On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > >> > > Probably not a big deal, just for corretness. > >> > > > >> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > >> > > --- > >> > > net/ipv4/ipmr.c | 2 +- > >> > > net/ipv6/ip6mr.c | 2 +- > >> > > 2 files changed, 2 insertions(+), 2 deletions(-) > >> > > > >> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > >> > > index d6fede8..68f67b8 100644 > >> > > --- a/net/ipv4/ipmr.c > >> > > +++ b/net/ipv4/ipmr.c > >> > > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net > >> > > *net) > >> > > > >> > > rtnl_lock(); > >> > > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { > >> > > - list_del(&mrt->list); > >> > > + list_del_rcu(&mrt->list); > >> > > ipmr_free_table(mrt); > >> > > } > >> > > rtnl_unlock(); > >> > > >> > I really do wonder if we have the rcu locking correct in there: > >> > > >> > Looking into getsockopt/setsockopt operations, we might have socket > >> > lock, but I cannot see where we lock rcu, so the ipmr_get_table call is > >> > safe. Do you also see the problem? > > I see only ipmr_rule_action() really has RCU read lock, I think the > sockopt > operations should take RCU read lock too, since it is supposed to be > protected by rcu+rtnl rather than rcu+sock lock? Exactly, it would be canonical, especially because of lockdep. I am not sure yet about socket lock, but will review that tomorrow. Maybe we also have to add __rcu annotations? Bye, Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-26 1:01 ` Hannes Frederic Sowa @ 2015-03-26 12:23 ` Hannes Frederic Sowa 2015-03-31 0:17 ` Cong Wang 0 siblings, 1 reply; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-26 12:23 UTC (permalink / raw) To: Cong Wang; +Cc: Cong Wang, netdev On Thu, Mar 26, 2015, at 02:01, Hannes Frederic Sowa wrote: > On Wed, Mar 25, 2015, at 23:33, Cong Wang wrote: > > On Wed, Mar 25, 2015 at 1:56 PM, Hannes Frederic Sowa > > <hannes@stressinduktion.org> wrote: > > > On Wed, Mar 25, 2015, at 21:21, Hannes Frederic Sowa wrote: > > >> On Wed, Mar 25, 2015, at 21:07, Hannes Frederic Sowa wrote: > > >> > On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > > >> > > Probably not a big deal, just for corretness. > > >> > > > > >> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > >> > > --- > > >> > > net/ipv4/ipmr.c | 2 +- > > >> > > net/ipv6/ip6mr.c | 2 +- > > >> > > 2 files changed, 2 insertions(+), 2 deletions(-) > > >> > > > > >> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > > >> > > index d6fede8..68f67b8 100644 > > >> > > --- a/net/ipv4/ipmr.c > > >> > > +++ b/net/ipv4/ipmr.c > > >> > > @@ -280,7 +280,7 @@ static void __net_exit ipmr_rules_exit(struct net > > >> > > *net) > > >> > > > > >> > > rtnl_lock(); > > >> > > list_for_each_entry_safe(mrt, next, &net->ipv4.mr_tables, list) { > > >> > > - list_del(&mrt->list); > > >> > > + list_del_rcu(&mrt->list); > > >> > > ipmr_free_table(mrt); > > >> > > } > > >> > > rtnl_unlock(); > > >> > > > >> > I really do wonder if we have the rcu locking correct in there: > > >> > > > >> > Looking into getsockopt/setsockopt operations, we might have socket > > >> > lock, but I cannot see where we lock rcu, so the ipmr_get_table call is > > >> > safe. Do you also see the problem? > > > > I see only ipmr_rule_action() really has RCU read lock, I think the > > sockopt > > operations should take RCU read lock too, since it is supposed to be > > protected by rcu+rtnl rather than rcu+sock lock? > > Exactly, it would be canonical, especially because of lockdep. I am not > sure yet about socket lock, but will review that tomorrow. The rest of the locking seems to be fine. So only rcu_read_lock() is missing for lockdep correctness. Bye, Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list 2015-03-26 12:23 ` Hannes Frederic Sowa @ 2015-03-31 0:17 ` Cong Wang 0 siblings, 0 replies; 10+ messages in thread From: Cong Wang @ 2015-03-31 0:17 UTC (permalink / raw) To: Hannes Frederic Sowa; +Cc: Cong Wang, netdev On Thu, Mar 26, 2015 at 5:23 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > The rest of the locking seems to be fine. So only rcu_read_lock() is > missing for lockdep correctness. > Since DaveM already marks this as "Changes requested", I will: 1) separate patch 1/2 out for net and possibly stable; 2) fold patch 2/2 into a more complete patch including the rcu_read_lock() you mentioned, only for net-next. Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup 2015-03-25 19:05 [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Cong Wang 2015-03-25 19:05 ` [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list Cong Wang @ 2015-03-25 20:23 ` Hannes Frederic Sowa 1 sibling, 0 replies; 10+ messages in thread From: Hannes Frederic Sowa @ 2015-03-25 20:23 UTC (permalink / raw) To: Cong Wang, netdev On Wed, Mar 25, 2015, at 20:05, Cong Wang wrote: > This is the IPv4 part for commit 905a6f96a1b1 > (ipv6: take rtnl_lock and mark mrt6 table as freed on namespace cleanup). > > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-03-31 0:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-25 19:05 [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Cong Wang 2015-03-25 19:05 ` [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list Cong Wang 2015-03-25 20:07 ` Hannes Frederic Sowa 2015-03-25 20:21 ` Hannes Frederic Sowa 2015-03-25 20:56 ` Hannes Frederic Sowa 2015-03-25 22:33 ` Cong Wang 2015-03-26 1:01 ` Hannes Frederic Sowa 2015-03-26 12:23 ` Hannes Frederic Sowa 2015-03-31 0:17 ` Cong Wang 2015-03-25 20:23 ` [Patch net 1/2] ipv4: take rtnl_lock and mark mrt table as freed on namespace cleanup Hannes Frederic Sowa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).