On 29.12.2017 21:18, Eric W. Biederman wrote:
> Kirill Tkhai <ktk...@virtuozzo.com> 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" <ebied...@xmission.com>
>
> 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.
You provided comments on my changes and you asked couple of questions. I replied
your questions and explained, why it seems important to made the first hunk.
Since
there were questions from you I interpreted the conversation is a discussion.
Later
there was no an answer from you, and patchset status became not clear for me,
and I
wrote about that. I had no an aim to disappoint you or ignore your position.
Thank you for the reply. Now the position is clear for me. I'll remove the first
hunk and resend the changed patchset like you suggested.
Kirill
>> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
>> ---
>> 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);
