* [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 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.