Re: [PATCH net v2] tipc: purge cong_links under the socket lock in tipc_release()

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

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

- [High] Incomplete fix: the commit message's central invariant — "Every
  other accessor of that list runs under the socket lock" — is factually…

--

tipc: purge cong_links under the socket lock in tipc_release()

This moves tipc_dest_list_purge(&tsk->cong_links) from after release_sock()
to before sock_orphan()/release_sock() in tipc_release(), so the purge runs
while the closing task owns the socket lock.  The stated goal is to exclude a
concurrent SOCK_WAKEUP handler that reaches tipc_dest_del() on the same list
via tipc_sk_rcv()'s spin_trylock_bh().

[Severity: High]
The commit message says:

  "Every other accessor of that list runs under the socket lock"

Is that accurate for tipc_sk_timeout()?  In net/tipc/socket.c it touches
cong_links after dropping the socket spinlock:

net/tipc/socket.c:tipc_sk_timeout() {
	...
	bh_unlock_sock(sk);

	if (!skb_queue_empty(&list))
		rc = tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);

	/* SYN messages may cause link congestion */
	if (rc == -ELINKCONG) {
		tipc_dest_push(&tsk->cong_links, pnode, 0);
		tsk->cong_link_cnt = 1;
	}
	sock_put(sk);
}

Here neither lock_sock() ownership nor sk->sk_lock.slock is held, and
tipc_dest_push() walks the list with tipc_dest_find() and then list_add()s,
with no internal locking:

net/tipc/name_table.c:tipc_dest_push() {
	if (tipc_dest_find(l, node, port))
		return false;

	dst = kmalloc_obj(*dst, GFP_ATOMIC);
	...
	list_add(&dst->list, l);
}

Does sk_stop_timer() on the preceding line quiesce that path?  It is the
asynchronous variant:

net/core/sock.c:sk_stop_timer() {
	if (timer_delete(timer))
		__sock_put(sk);
}

timer_delete() does not wait for a callback that is already running, and the
timer callback holds its own socket reference, so a tipc_sk_timeout() already
past bh_unlock_sock() and inside tipc_node_xmit() keeps running.

So can this interleaving happen on a connecting/established socket?

CPU0 (timer)                          CPU1 (close)
tipc_sk_timeout()
  bh_lock_sock(sk)
  sock_owned_by_user() false
  tipc_sk_retry_connect()
  bh_unlock_sock(sk)
  tipc_node_xmit()  /* -ELINKCONG */
                                      lock_sock(sk)
                                      __tipc_shutdown()
                                      sk_stop_timer()  /* nothing pending */
                                      tipc_sk_remove()
                                      tipc_dest_list_purge(&tsk->cong_links)
  tipc_dest_push(&tsk->cong_links, ...)

If CPU0's tipc_dest_find() walk interleaves with the purge's
list_for_each_entry_safe { list_del; kfree; }, does this reproduce the same
slab-use-after-free / __list_del_entry_valid_or_report signature quoted in the
changelog, only reached through the timer accessor instead of the SOCK_WAKEUP
accessor?

And if the push lands strictly after the purge, what frees that entry?
tipc_release() has no second purge after this change, tipc_sk_callback() only
does sock_put(), and tipc_sock_destruct() purges only sk_receive_queue:

net/tipc/socket.c:tipc_sk_callback() {
	sock_put(&tsk->sk);
}

net/tipc/socket.c:tipc_sock_destruct() {
	__skb_queue_purge(&sk->sk_receive_queue);
}

Does that leak the 32-byte struct tipc_dest and leave tsk->cong_link_cnt = 0
disagreeing with a still-populated list head that is freed with tsk?  The
unlocked tsk->cong_link_cnt = 1 in tipc_sk_timeout() also races the unlocked
reset after release_sock().

The unlocked timer accessor is not introduced here, but moving the purge
earlier widens the post-purge interval by sock_orphan() plus the
release_sock() backlog drain, and the safety argument rests on an invariant
this path does not satisfy.

Would it make sense either to do the timer's tipc_node_xmit() /
tipc_dest_push() / cong_link_cnt accounting under the socket lock, or to
quiesce the timer with a synchronous cancellation before purging?  Failing
that, could the changelog and the new comment be narrowed so they do not claim
every other accessor is serialised by the socket lock?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902111847.79955-1-juny24602%40gmail.com