Re: [PATCH net v2 2/2] tipc: make conn_idr teardown safe

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] tipc: priority inversion deadlock via cond_resched() in
  tipc_topsrv_stop()

Pre-existing issues:
- [High] NULL pointer dereference of con->sock in tipc_conn_close() when
  the teardown walk in tipc_topsrv_stop() reaches a socket-less…

--

tipc: make conn_idr teardown safe

The teardown walk in tipc_topsrv_stop() now iterates conn_idr with
idr_get_next() instead of incrementing a numeric ID, drops idr_lock and
reschedules when no entry can be taken, and switches from conn_get() to
kref_get_unless_zero() so an entry already being released is not revived.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 908622a3d0fca..9333e36a74de7 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -710,15 +710,20 @@ static void tipc_topsrv_stop(struct net *net)
>  	cancel_work_sync(&srv->awork);
>  
>  	spin_lock_bh(&srv->idr_lock);
> -	for (id = 0; srv->idr_in_use; id++) {
> -		con = idr_find(&srv->conn_idr, id);
> -		if (con) {
> -			conn_get(con);
> +	for (id = 0; srv->idr_in_use;) {
> +		con = idr_get_next(&srv->conn_idr, &id);
> +		if (!con || !kref_get_unless_zero(&con->kref)) {
>  			spin_unlock_bh(&srv->idr_lock);
> -			tipc_conn_close(con);
> -			conn_put(con);
> +			cond_resched();
>  			spin_lock_bh(&srv->idr_lock);
> +			id = 0;
> +			continue;
>  		}
> +		id++;
> +		spin_unlock_bh(&srv->idr_lock);
> +		tipc_conn_close(con);
> +		conn_put(con);
> +		spin_lock_bh(&srv->idr_lock);
>  	}

[Severity: High]
The not-taken branch turns this walk into a poll loop whose only
forward-progress mechanism is cond_resched(). When idr_get_next()
returns NULL, or when kref_get_unless_zero() fails because the entry is
already on its way out, we drop idr_lock, call cond_resched(), reset id
to 0 and start over, and the loop only terminates once srv->idr_in_use
reaches zero. That counter is decremented in exactly one place:

net/tipc/topsrv.c:tipc_conn_kref_release() {
	spin_lock_bh(&s->idr_lock);
	idr_remove(&s->conn_idr, con->conid);
	s->idr_in_use--;
	spin_unlock_bh(&s->idr_lock);
	...
}

and in the window this branch is spinning in, the last reference is
typically held by a queued rcv_wq/send_wq item (tipc_conn_recv_work()
or tipc_conn_send_work()), i.e. by a SCHED_OTHER kworker. Note that
tipc_topsrv_work_stop() only runs after the walk, so both queues are
still live at this point, and nothing before the walk flushes them --
only srv->awork is cancelled.

So the exit condition of this loop depends on a lower-priority thread
being scheduled. cond_resched() does not yield to lower-priority tasks,
and on full-preemption builds it degenerates into a no-op, so if
tipc_topsrv_stop() is entered from an RT task (netns exit and module
unload can both be driven from a SCHED_FIFO context), on a single CPU
we spin taking and dropping idr_lock forever while the kworker that
would run tipc_conn_kref_release() and drop idr_in_use never gets to
run. That is a livelock rather than a latency glitch. The old
idr_find() loop had its own unbounded-scan problem, but it was not
structured as a retry-from-zero wait, so the patch makes cond_resched()
load-bearing as a synchronisation primitive, which it is not.

What is the intended guarantee here? If the walk really has to wait for
asynchronous releases, a sleeping wait looks more appropriate -- e.g. a
wait_event()/completion signalled from tipc_conn_kref_release() when
idr_in_use hits zero. Alternatively, would it be cleaner to tear down
rcv_wq/send_wq (tipc_topsrv_work_stop()) before walking conn_idr, so
that no async reference can still be outstanding and the loop needs no
retry branch at all? Could you spell out which of these you consider
correct, or why cond_resched() is sufficient in your view?

[Severity: High]
This isn't a bug introduced by this patch, since the old idr_find() plus
conn_get() walk fed tipc_conn_close() the same way, but can this walk hand
a socket-less conn_idr entry to tipc_conn_close() and dereference NULL?

Kernel subscriptions are published into conn_idr with sock == NULL:

net/tipc/topsrv.c:tipc_topsrv_kern_subscr() {
	...
	con = tipc_conn_alloc(tipc_topsrv(net), NULL);
	...
}

and two other users of con->sock treat it as optional:

net/tipc/topsrv.c:tipc_conn_kref_release() {
	...
	if (con->sock)
		sock_release(con->sock);
	...
}

but tipc_conn_close() dereferences it in its first statement, before
CF_CONNECTED is even tested:

net/tipc/topsrv.c:tipc_conn_close() {
	struct sock *sk = con->sock->sk;
	...
}

The ordering I think reaches this after the walk grabs a reference with
kref_get_unless_zero():

  tipc_group_create() -> tipc_topsrv_kern_subscr() creates conn X with
  sock == NULL

  a name table event runs tipc_topsrv_queue_evt(), which keeps its
  tipc_conn_lookup() reference when the work is queued:

	if (queue_work(srv->send_wq, &con->swork))
		return;

  socket close -> tipc_group_delete() -> tipc_topsrv_kern_unsubscr()
  clears CF_CONNECTED and drops its two references without waiting for
  the queued swork, so X stays in conn_idr owned by the pending work

  netns exit -> tipc_topsrv_exit_net() -> tipc_topsrv_stop() runs before
  send_wq has executed that work

Only srv->awork is cancelled before the walk, and send_wq/rcv_wq are
destroyed afterwards in tipc_topsrv_work_stop(), so is there anything that
excludes a still-queued swork keeping a sock == NULL entry alive here?
Would a check for con->sock in tipc_conn_close(), or skipping socket-less
entries in the walk, be appropriate so the three sites touching con->sock
agree?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789960909.git.xuyuqiabc%40gmail.com