From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH RESEND 1/3] net: Fix possible race in peernet2id_alloc() Date: Fri, 29 Dec 2017 12:18:33 -0600 Message-ID: <87y3ll2y9y.fsf@xmission.com> References: <151453250786.12258.8455863810071017385.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain Cc: netdev@vger.kernel.org, davem@davemloft.net, eric.dumazet@gmail.com To: Kirill Tkhai Return-path: Received: from out03.mta.xmission.com ([166.70.13.233]:38499 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751200AbdL2STK (ORCPT ); Fri, 29 Dec 2017 13:19:10 -0500 In-Reply-To: <151453250786.12258.8455863810071017385.stgit@localhost.localdomain> (Kirill Tkhai's message of "Fri, 29 Dec 2017 10:29:43 +0300") Sender: netdev-owner@vger.kernel.org List-ID: Kirill Tkhai writes: > peernet2id_alloc() is racy without rtnl_lock() as atomic_read(&peer->count) > under net->nsid_lock does not guarantee, peer is alive: > > rcu_read_lock() > peernet2id_alloc() .. > spin_lock_bh(&net->nsid_lock) .. > atomic_read(&peer->count) == 1 .. > .. put_net() > .. cleanup_net() > .. for_each_net(tmp) > .. spin_lock_bh(&tmp->nsid_lock) > .. __peernet2id(tmp, net) == -1 > .. .. > .. .. > __peernet2id_alloc(alloc == true) .. > .. .. > rcu_read_unlock() .. > .. synchronize_rcu() > .. kmem_cache_free(net) > > After the above situation, net::netns_id contains id pointing to freed memory, > and any other dereferencing by the id will operate with this freed memory. > > Currently, peernet2id_alloc() is used under rtnl_lock() everywhere except > ovs_vport_cmd_fill_info(), and this race can't occur. But peernet2id_alloc() > is generic interface, and better we fix it before someone really starts > use it in wrong context. Nacked-by: "Eric W. Biederman" I have already made a clear objection to the first unnecessary and confusing hunk. Simply resending the muddle headed code doesn't make it better. Eric > Signed-off-by: Kirill Tkhai > --- > net/core/net_namespace.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c > index 60a71be75aea..6a4eab438221 100644 > --- a/net/core/net_namespace.c > +++ b/net/core/net_namespace.c > @@ -221,17 +221,32 @@ static void rtnl_net_notifyid(struct net *net, int cmd, int id); > */ > int peernet2id_alloc(struct net *net, struct net *peer) > { > - bool alloc; > + bool alloc = false, alive = false; > int id; > > - if (atomic_read(&net->count) == 0) > - return NETNSA_NSID_NOT_ASSIGNED; > spin_lock_bh(&net->nsid_lock); > - alloc = atomic_read(&peer->count) == 0 ? false : true; > + /* Spinlock guarantees we never hash a peer to net->netns_ids > + * after idr_destroy(&net->netns_ids) occurs in cleanup_net(). > + */ > + if (atomic_read(&net->count) == 0) { > + id = NETNSA_NSID_NOT_ASSIGNED; > + goto unlock; > + } > + /* > + * When peer is obtained from RCU lists, we may race with > + * its cleanup. Check whether it's alive, and this guarantees > + * we never hash a peer back to net->netns_ids, after it has > + * just been idr_remove()'d from there in cleanup_net(). > + */ > + if (maybe_get_net(peer)) > + alive = alloc = true; > id = __peernet2id_alloc(net, peer, &alloc); > +unlock: > spin_unlock_bh(&net->nsid_lock); > if (alloc && id >= 0) > rtnl_net_notifyid(net, RTM_NEWNSID, id); > + if (alive) > + put_net(peer); > return id; > } > EXPORT_SYMBOL_GPL(peernet2id_alloc);