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

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

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

tipc_release() frees the elements of tsk->cong_links after release_sock(),
i.e. with no lock held:

	tipc_sk_remove(tsk);
	sock_orphan(sk);
	release_sock(sk);
	tipc_dest_list_purge(&tsk->cong_links);	/* no lock */
	tsk->cong_link_cnt = 0;

Every other accessor of that list runs under the socket lock, including
the SOCK_WAKEUP handler in tipc_sk_proto_rcv(), which does
tipc_dest_del(&tsk->cong_links, ...) while holding only sk->sk_lock.slock
via tipc_sk_rcv()'s spin_trylock_bh().  Because the purge never acquires
that spinlock, it provides no mutual exclusion against the wakeup path.

A SOCK_WAKEUP delivered for this port can therefore run concurrently with
the purge: tipc_sk_rcv() looks the socket up and takes a reference before
tipc_sk_remove() unhashes it, is then delayed past release_sock() so
sock_owned_by_user() is false, its spin_trylock_bh() succeeds, and it
list_del()s and kfree()s a struct tipc_dest that the closing task is
walking at the same time.  Both paths free entries of the same list.

__tipc_shutdown() does not close this window: it waits on
!tsk->cong_link_cnt but ignores the return value of tipc_wait_for_cond(),
which returns early on timeout, on a pending signal, or on sk_err, so the
close can proceed with cong_links still populated.

  BUG: KASAN: slab-use-after-free in
   __list_del_entry_valid_or_report (lib/list_debug.c:49)
  Read of size 8 at addr ffff888028b4b6c8 by task poc_cong_race/9425
   __list_del_entry_valid_or_report (lib/list_debug.c:49)
   tipc_dest_list_purge (net/tipc/name_table.c:1225)
   tipc_release (net/tipc/socket.c:653)
   __sock_release (net/socket.c:735)
   sock_close (net/socket.c:1526)
   __x64_sys_close (fs/open.c:1560)
  Allocated by task 9425:
   tipc_dest_push (net/tipc/name_table.c:1183)
   __tipc_sendmsg (net/tipc/socket.c:1518)
  Freed by task 9418:
   kfree (mm/slub.c:6792)
   tipc_dest_del (net/tipc/name_table.c:1216)
   tipc_sk_filter_rcv (net/tipc/socket.c:2164)
   tipc_sk_rcv (net/tipc/socket.c:2450)
   tipc_rcv (net/tipc/node.c:2210)
   tipc_udp_recv (net/tipc/udp_media.c:389)
  kmalloc-32, freed 32-byte region [ffff888028b4b6c0, ffff888028b4b6e0)
  Oops: general protection fault, probably for non-canonical address
   0xe0347c4420000499
  Kernel panic - not syncing: Fatal exception

Move the purge above release_sock() so it runs under the socket lock, the
same discipline commit 844cf763fba6 ("tipc: make macro tipc_wait_for_cond()
smp safe") established for the wait condition.  A concurrent tipc_sk_rcv()
then either backlogs the wakeup because the socket is owned, or processes
it against an already empty list, and no new delivery can arrive because
tipc_sk_remove() has already unhashed the socket.  tsk->cong_link_cnt is
left where it is, so a wakeup taken from the backlog by release_sock()
still decrements the real count.

Fixes: 365ad353c256 ("tipc: reduce risk of user starvation during link congestion")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:hy4
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

v2:
 - Move only the purge.  v1 moved "tsk->cong_link_cnt = 0" above
   release_sock() as well, which makes a SOCK_WAKEUP taken from the backlog
   by release_sock() decrement an already-zeroed u16 down to 65535.  The
   value is dead by then, but the race does not need that hunk.

v1: https://lore.kernel.org/netdev/20260731101926.31514-1-juny24602@gmail.com/

 net/tipc/socket.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d5d70eb230b5..cf7ee31638ab 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -647,10 +647,14 @@ static int tipc_release(struct socket *sock)
 	sk_stop_timer(sk, &sk->sk_timer);
 	tipc_sk_remove(tsk);
 
+	/* Purge under the socket lock: a SOCK_WAKEUP looked up before
+	 * tipc_sk_remove() can still reach tipc_dest_del() on this list.
+	 */
+	tipc_dest_list_purge(&tsk->cong_links);
+
 	sock_orphan(sk);
 	/* Reject any messages that accumulated in backlog queue */
 	release_sock(sk);
-	tipc_dest_list_purge(&tsk->cong_links);
 	tsk->cong_link_cnt = 0;
 	call_rcu(&tsk->rcu, tipc_sk_callback);
 	sock->sk = NULL;
-- 
2.55.0

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

[netdev-bot+sashiko]

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().

> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index d5d70eb230b56..cf7ee31638abf 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -647,10 +647,14 @@ static int tipc_release(struct socket *sock)
>  	sk_stop_timer(sk, &sk->sk_timer);
>  	tipc_sk_remove(tsk);
>  
> +	/* Purge under the socket lock: a SOCK_WAKEUP looked up before
> +	 * tipc_sk_remove() can still reach tipc_dest_del() on this list.
> +	 */
> +	tipc_dest_list_purge(&tsk->cong_links);
> +
>  	sock_orphan(sk);
>  	/* Reject any messages that accumulated in backlog queue */
>  	release_sock(sk);
> -	tipc_dest_list_purge(&tsk->cong_links);
>  	tsk->cong_link_cnt = 0;
>  	call_rcu(&tsk->rcu, tipc_sk_callback);
>  	sock->sk = NULL;

[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