Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr table from list

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

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