Le 19/12/2017 à 18:27, Eric W. Biederman a écrit :
>
> (I can trivially verify that that idr_remove in cleanup_net happens
> after the network namespace count has dropped to zero --EWB)
>
> Function get_net_ns_by_id() does not check for net::count
> after it has found a peer in netns_ids idr.
>
> It may dereference a peer, after its count has already been
> finaly decremented. This leads to double free and memory
> corruption:
>
> put_net(peer) rtnl_lock()
> atomic_dec_and_test(&peer->count) [count=0] ...
> __put_net(peer) get_net_ns_by_id(net, id)
> spin_lock(&cleanup_list_lock)
> list_add(&net->cleanup_list, &cleanup_list)
> spin_unlock(&cleanup_list_lock)
> queue_work() peer =
> idr_find(&net->netns_ids, id)
> | get_net(peer) [count=1]
> | ...
> | (use after final put)
> v ...
> cleanup_net() ...
> spin_lock(&cleanup_list_lock) ...
> list_replace_init(&cleanup_list, ..) ...
> spin_unlock(&cleanup_list_lock) ...
> ... ...
> ... put_net(peer)
> ...
> atomic_dec_and_test(&peer->count) [count=0]
> ...
> spin_lock(&cleanup_list_lock)
> ...
> list_add(&net->cleanup_list, &cleanup_list)
> ...
> spin_unlock(&cleanup_list_lock)
> ... queue_work()
> ... rtnl_unlock()
> rtnl_lock() ...
> for_each_net(tmp) { ...
> id = __peernet2id(tmp, peer) ...
> spin_lock_irq(&tmp->nsid_lock) ...
> idr_remove(&tmp->netns_ids, id) ...
> ... ...
> net_drop_ns() ...
> net_free(peer) ...
> } ...
> |
> v
> cleanup_net()
> ...
> (Second free of peer)
>
> Also, put_net() on the right cpu may reorder with left's cpu
> list_replace_init(&cleanup_list, ..), and then cleanup_list
> will be corrupted.
>
> Since cleanup_net() is executed in worker thread, while
> put_net(peer) can happen everywhere, there should be
> enough time for concurrent get_net_ns_by_id() to pick
> the peer up, and the race does not seem to be unlikely.
> The patch fixes the problem in standard way.
>
> (Also, there is possible problem in peernet2id_alloc(), which requires
> check for net::count under nsid_lock and maybe_get_net(peer), but
> in current stable kernel it's used under rtnl_lock() and it has to be
> safe. Openswitch begun to use peernet2id_alloc(), and possibly it should
> be fixed too. While this is not in stable kernel yet, so I'll send
> a separate message to netdev@ later).
>
> Cc: Nicolas Dichtel <nicolas.dich...@6wind.com>
> Signed-off-by: Kirill Tkhai <ktk...@virtuozzo.com>
> Fixes: 0c7aecd4bde4 "netns: add rtnl cmd to add and get peer netns ids"
> Reviewed-by: Andrey Ryabinin <aryabi...@virtuozzo.com>
> Reviewed-by: "Eric W. Biederman" <ebied...@xmission.com>
> Signed-off-by: Eric W. Biederman <ebied...@xmission.com>
Good catch, thank you.
Acked-by: Nicolas Dichtel <nicolas.dich...@6wind.com>
