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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 25 Mar 2015 21:56:44 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Subject: Re: [Patch net 2/2] ipmr,ip6mr: call list_del_rcu() when deleting mr
 table from list

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@...il.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@...essinduktion.org>

Thanks,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html