| Message ID | 1427310345-29829-2-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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();
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(-)