Re: [PATCH] tipc: fix use-after-free in netns teardown

[Jérémy Jean]

On 2026-10-04 21:27, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [Severity: Medium]
>
> Can restarting from id 0 on every pass livelock the teardown?
>
> rcv_wq is an ordered workqueue:
>
> tipc_topsrv_work_start()
>     s->rcv_wq = alloc_ordered_workqueue("tipc_rcv", 0);
>
> Take a lower-id connection A whose rwork is queued behind the running
> rwork of a higher-id connection B. The loop closes A, but the reference
> held by A's queued rwork keeps A in conn_idr. On every later pass
> idr_get_next() returns A again, so the loop never reaches B.
>
> tipc_conn_recv_work() keeps running while B is connected and requests
> keep arriving:
>
>     while (connected(con)) {
>         if (tipc_conn_rcv_from_sock(con))
>             break;
>     ...
>
> The cond_resched() here gives up the CPU. A's work still cannot run on
> the ordered queue until B's worker returns. If B's receive queue never
> empties, idr_in_use never reaches 0 and cleanup_net() spins.
>
> The old loop advanced with id++. It would have cleared CF_CONNECTED on 
> B,
> which ends B's worker.
>
> Local peers probably cannot keep this going. The listener is bound with
> TIPC_NODE_SCOPE, and local user sockets hold a reference on the netns.
>
> Remote peers are another matter. tipc_net_ops is registered before
> tipc_topsrv_net_ops, so the bearers are still up when 
> tipc_topsrv_stop()
> runs. The TIPC_LISTEN branch of tipc_sk_filter_connect() has no scope
> check. A remote node that knows the listener's port id might be able to
> connect and keep sending TIPC_SUB_CANCEL requests. That precondition 
> has
> not been confirmed.
>
> Would it work to first walk forward through all entries, using
> idr_get_next() from id + 1 and closing each one? The loop could then
> retry from 0 only for the entries still waiting on asynchronous 
> release.

This looks like a valid concern. I will address it in a v2.

> > +		if (con && !kref_get_unless_zero(&con->kref))
> > +			con = NULL;
> > +		spin_unlock_bh(&srv->idr_lock);
> >  		if (con) {
> > -			conn_get(con);
> > -			spin_unlock_bh(&srv->idr_lock);
> >  			tipc_conn_close(con);
>
> [Severity: High]
>
> This is a pre-existing issue, but can this call to tipc_conn_close()
> dereference a NULL con->sock?

Indeed, Sashiko is right. I reproduced it.

I will fix it in a seperate commit in the v2, by guarding 
tipc_conn_close()
behind a check on con->sock:

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index af530c9..01dd34b 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -706,7 +706,8 @@ static void tipc_topsrv_stop(struct net *net)
                if (con) {
                        conn_get(con);
                        spin_unlock_bh(&srv->idr_lock);
-                       tipc_conn_close(con);
+                       if (con->sock)
+                               tipc_conn_close(con);
                        conn_put(con);
                        spin_lock_bh(&srv->idr_lock);
                }


> tipc_topsrv_kern_subscr() creates conn_idr entries that have no socket:
>
>     con = tipc_conn_alloc(tipc_topsrv(net), NULL);
>
> tipc_conn_close() dereferences the socket before it tests CF_CONNECTED:
>
>     static void tipc_conn_close(struct tipc_conn *con)
>     {
>         struct sock *sk = con->sock->sk;
>
> Other code in this file already allows for a NULL sock.
> tipc_conn_kref_release() checks if (con->sock). 
> tipc_conn_send_to_sock()
> only calls tipc_conn_close() inside its if (con->sock) branch.
>
> Here is a possible sequence:
>
> 1. tipc_topsrv_queue_evt() takes a reference on a kernel subscription
>    connection K through tipc_conn_lookup(). It then queues K->swork on
>    send_wq.
>
> 2. Before that work runs, the group socket is closed:
>    tipc_release()->tipc_group_delete()->tipc_topsrv_kern_unsubscr().
>    This clears CF_CONNECTED and drops two references. K stays in
>    conn_idr, held only by the swork reference.
>
> 3. cleanup_net()->tipc_topsrv_exit_net()->tipc_topsrv_stop() runs.
>    cancel_work_sync(&srv->awork) does not drain send_wq. idr_get_next()
>    returns K, kref_get_unless_zero() succeeds, and tipc_conn_close(K)
>    dereferences NULL.
>
> The old idr_find() plus conn_get() loop had the same exposure. The new
> loop retries until idr_in_use reaches 0, so any socketless entry still 
> in
> conn_idr is now guaranteed to reach tipc_conn_close() at least once.
>
> This loop is being rewritten anyway. Could it skip tipc_conn_close() 
> when
> con->sock is NULL? Alternatively, tipc_conn_close() could check
> con->sock before using it.
>
> >  			conn_put(con);
> > -			spin_lock_bh(&srv->idr_lock);
> >  		}
> > +		cond_resched();
> > +		spin_lock_bh(&srv->idr_lock);
> >  	}
>
> [ ... ]