[PATCH v2 RESEND] IB/isert: tear connections down from an ib_client on device removal

[PATCH v2 RESEND] IB/isert: tear connections down from an ib_client on device removal

[Yehyeong Lee]

The RDMA_CM_EVENT_DEVICE_REMOVAL callback waits for the connection to
reach ISER_CONN_DOWN and then frees it:

	wait_event_interruptible(isert_conn->rem_wait,
				 isert_conn->state == ISER_CONN_DOWN);
	kfree(isert_conn);

That is a use-after-free.  isert_release_work() publishes ISER_CONN_DOWN
before the last reference is dropped, so the condition is already true
when isert_connect_release() starts and the callback frees isert_conn
while isert_connect_release() is still dereferencing it.  Publishing the
state next to the wake-up instead does not close it either, because the
condition is evaluated without any lock, so the waiter can leave the wait
and free isert_conn while the releasing thread is still inside
wake_up_interruptible() taking isert_conn->rem_wait.lock.

The wait also deadlocks.  cma_send_device_removal_put() runs the callback
with the cm_id's handler_mutex held and rdma_destroy_id() takes that same
mutex, so a teardown that has already passed the isert_conn->dev_removed
test in isert_connect_release() blocks there while the callback waits for
that teardown to publish ISER_CONN_DOWN.  Nothing in the callback can
test whether the teardown has reached that point.

Tear the connections down from an ib_client removal callback instead,
where no cm_id lock is held, and wait there for the last isert_device
reference, which isert_connect_release() drops once it is done with the
device.  nvmet-rdma fixed the same two problems the same way, see
commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not deadlock in
device removal").

The cm callback now returns zero for RDMA_CM_EVENT_DEVICE_REMOVAL and
keeps the cm_id, which leaves isert_connect_release() as the only place
that destroys it, so isert_conn->dev_removed and isert_conn->rem_wait are
gone.  A connection that never reached RDMA_CM_EVENT_ESTABLISHED now
delays the removal until the CM reports RDMA_CM_EVENT_UNREACHABLE and
isert_connect_error() drops the last reference.  Before this patch that
case blocked for good, because that CM event needs the handler_mutex the
waiting callback holds.

Measured with soft-RoCE under KASAN on a local device removal racing 32
connections: 11 of 20 unpatched runs hit the splat below and 0 of 20
patched runs did, and the removal itself finished in all 20 patched runs
and in none of the unpatched ones.  With the wait kept and its window
widened, 3 of 3 runs deadlocked and 0 of 3 did with this patch.  Normal
login and I/O passed 24 of 30 runs on either side of the patch.

  BUG: KASAN: slab-use-after-free in isert_connect_release+0x5cb/0x620
  Read of size 4 at addr ffff88810acbd158 by task kworker/u16:5/70

  CPU: 1 UID: 0 PID: 70 Comm: kworker/u16:5 Not tainted 7.3.0-CTL2-13247-g330517c63aa9 #1 PREEMPT(full)
  Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  Workqueue: isert_release_wq isert_release_work
  Call Trace:
   <TASK>
   dump_stack_lvl+0x53/0x70
   print_report+0xd0/0x630
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? kfree+0x121/0x390
   ? isert_connect_release+0x5cb/0x620
   kasan_report+0xce/0x100
   ? isert_connect_release+0x5cb/0x620
   isert_connect_release+0x5cb/0x620
   process_one_work+0x697/0x1060
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0xcb/0xe0
   ? __pfx_isert_release_work+0x10/0x10
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x2c8/0x3b0
   ? recalc_sigpending+0x15c/0x1e0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x36e/0x5a0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x572/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>

  Allocated by task 81:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x148/0x360
   isert_cma_handler+0x1e3/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_ib_req_handler+0x17d9/0x4480
   cm_process_work+0x41/0x330
   cm_work_handler+0x5727/0xc160
   process_one_work+0x697/0x1060
   worker_thread+0x45b/0xd10
   kthread+0x2c8/0x3b0
   ret_from_fork+0x36e/0x5a0
   ret_from_fork_asm+0x1a/0x30

  Freed by task 218:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x121/0x390
   isert_cma_handler+0x10ff/0x2840
   cma_cm_event_handler+0x3e/0x240
   cma_process_remove+0xff/0x5a0
   cma_remove_one+0xcc/0x250
   remove_client_context+0xa4/0xf0
   disable_device+0x128/0x240
   __ib_unregister_device+0x226/0x3f0
   ib_unregister_device_and_put+0x50/0x70
   nldev_dellink+0x28e/0x3d0
   rdma_nl_rcv_msg+0x29c/0x4e0
   rdma_nl_rcv_skb.constprop.0.isra.0+0x21d/0x370
   netlink_unicast+0x68f/0x940
   netlink_sendmsg+0x810/0xf20
   __sys_sendto+0x337/0x390
   __x64_sys_sendto+0xdb/0x1b0
   do_syscall_64+0xdd/0x4a0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: 63b268d232b8 ("IB/isert: Properly release resources on DEVICE_REMOVAL")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Resending as a standalone thread; the v2 I sent earlier went out
In-Reply-To v1 by mistake. No code changes. Apologies for the noise.

v2: redesigned to tear down from an ib_client (v1 had a handler_mutex
deadlock and did not close the rem_wait UAF).

 drivers/infiniband/ulp/isert/ib_isert.c | 129 +++++++++++++++++++-----
 drivers/infiniband/ulp/isert/ib_isert.h |   4 +-
 2 files changed, 103 insertions(+), 30 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index efa8d7b7844c9..35e4b9c917bd6 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -43,6 +43,8 @@ MODULE_PARM_DESC(sg_tablesize,
 
 static DEFINE_MUTEX(device_list_mutex);
 static LIST_HEAD(device_list);
+static DEFINE_MUTEX(isert_conn_mutex);
+static LIST_HEAD(isert_conn_list);
 static struct workqueue_struct *isert_login_wq;
 static struct workqueue_struct *isert_comp_wq;
 static struct workqueue_struct *isert_release_wq;
@@ -255,6 +257,8 @@ isert_device_put(struct isert_device *device)
 	if (!device->refcount) {
 		isert_free_device_ib_res(device);
 		list_del(&device->dev_node);
+		if (device->removal_comp)
+			complete(device->removal_comp);
 		kfree(device);
 	}
 	mutex_unlock(&device_list_mutex);
@@ -309,7 +313,7 @@ isert_init_conn(struct isert_conn *isert_conn)
 	INIT_LIST_HEAD(&isert_conn->node);
 	init_completion(&isert_conn->login_comp);
 	init_completion(&isert_conn->login_req_comp);
-	init_waitqueue_head(&isert_conn->rem_wait);
+	INIT_LIST_HEAD(&isert_conn->conn_list);
 	kref_init(&isert_conn->kref);
 	atomic_set(&isert_conn->ctrl_comp_cnt, 0);
 	mutex_init(&isert_conn->mutex);
@@ -468,6 +472,10 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 	list_add_tail(&isert_conn->node, &isert_np->accepted);
 	mutex_unlock(&isert_np->mutex);
 
+	mutex_lock(&isert_conn_mutex);
+	list_add_tail(&isert_conn->conn_list, &isert_conn_list);
+	mutex_unlock(&isert_conn_mutex);
+
 	return 0;
 
 out_destroy_qp:
@@ -491,9 +499,12 @@ isert_connect_release(struct isert_conn *isert_conn)
 
 	BUG_ON(!device);
 
+	mutex_lock(&isert_conn_mutex);
+	list_del_init(&isert_conn->conn_list);
+	mutex_unlock(&isert_conn_mutex);
+
 	isert_free_rx_descriptors(isert_conn);
-	if (isert_conn->cm_id &&
-	    !isert_conn->dev_removed)
+	if (isert_conn->cm_id)
 		rdma_destroy_id(isert_conn->cm_id);
 
 	if (isert_conn->qp)
@@ -504,12 +515,13 @@ isert_connect_release(struct isert_conn *isert_conn)
 	if (isert_conn->login_desc)
 		isert_free_login_buf(isert_conn);
 
-	isert_device_put(device);
+	kfree(isert_conn);
 
-	if (isert_conn->dev_removed)
-		wake_up_interruptible(&isert_conn->rem_wait);
-	else
-		kfree(isert_conn);
+	/*
+	 * Releases isert_remove_one() once the last connection running on
+	 * this device is gone.
+	 */
+	isert_device_put(device);
 }
 
 static void
@@ -625,13 +637,10 @@ isert_np_cma_handler(struct isert_np *isert_np,
 	return -1;
 }
 
-static int
-isert_disconnected_handler(struct rdma_cm_id *cma_id,
-			   enum rdma_cm_event_type event)
+/* Must be called with isert_conn->mutex held. */
+static void
+isert_teardown_conn(struct isert_conn *isert_conn)
 {
-	struct isert_conn *isert_conn = cma_id->qp->qp_context;
-
-	mutex_lock(&isert_conn->mutex);
 	switch (isert_conn->state) {
 	case ISER_CONN_TERMINATING:
 		break;
@@ -648,6 +657,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
 		isert_warn("conn %p terminating in state %d\n",
 			   isert_conn, isert_conn->state);
 	}
+}
+
+static int
+isert_disconnected_handler(struct rdma_cm_id *cma_id,
+			   enum rdma_cm_event_type event)
+{
+	struct isert_conn *isert_conn = cma_id->qp->qp_context;
+
+	mutex_lock(&isert_conn->mutex);
+	isert_teardown_conn(isert_conn);
 	mutex_unlock(&isert_conn->mutex);
 
 	return 0;
@@ -674,7 +693,6 @@ static int
 isert_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 {
 	struct isert_np *isert_np = cma_id->context;
-	struct isert_conn *isert_conn;
 	int ret = 0;
 
 	isert_info("%s (%d): status %d id %p np %p\n",
@@ -695,21 +713,15 @@ isert_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 		break;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
 	case RDMA_CM_EVENT_DISCONNECTED:
-	case RDMA_CM_EVENT_TIMEWAIT_EXIT:  /* FALLTHRU */
-		ret = isert_disconnected_handler(cma_id, event->event);
-		break;
+	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
-		isert_conn = cma_id->qp->qp_context;
-		isert_conn->dev_removed = true;
-		isert_disconnected_handler(cma_id, event->event);
-		wait_event_interruptible(isert_conn->rem_wait,
-					 isert_conn->state == ISER_CONN_DOWN);
-		kfree(isert_conn);
 		/*
-		 * return non-zero from the callback to destroy
-		 * the rdma cm id
+		 * Device removal is driven by isert_remove_one().  Returning
+		 * zero here keeps the cm_id; isert_connect_release() destroys
+		 * it once the connection has been released.
 		 */
-		return 1;
+		ret = isert_disconnected_handler(cma_id, event->event);
+		break;
 	case RDMA_CM_EVENT_REJECTED:
 		isert_info("Connection rejected: %s\n",
 			   rdma_reject_msg(cma_id, event->status));
@@ -2689,8 +2701,60 @@ static struct iscsit_transport iser_target_transport = {
 	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
 };
 
+static void
+isert_remove_one(struct ib_device *ib_device, void *client_data)
+{
+	DECLARE_COMPLETION_ONSTACK(removal_comp);
+	struct isert_device *device, *found = NULL;
+	struct isert_conn *isert_conn;
+
+	/*
+	 * Not from the RDMA_CM_EVENT_DEVICE_REMOVAL callback: that one runs
+	 * with the cm_id's handler mutex held, which a teardown already
+	 * destroying that cm_id needs.  Same reason as
+	 * commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not
+	 * deadlock in device removal").
+	 */
+	mutex_lock(&device_list_mutex);
+	list_for_each_entry(device, &device_list, dev_node) {
+		if (device->ib_device != ib_device)
+			continue;
+		device->refcount++;
+		device->removal_comp = &removal_comp;
+		found = device;
+		break;
+	}
+	mutex_unlock(&device_list_mutex);
+
+	if (!found)
+		return;
+
+	mutex_lock(&isert_conn_mutex);
+	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
+		if (isert_conn->device != found)
+			continue;
+
+		isert_info("removing conn %p state %d\n",
+			   isert_conn, isert_conn->state);
+		mutex_lock(&isert_conn->mutex);
+		isert_teardown_conn(isert_conn);
+		mutex_unlock(&isert_conn->mutex);
+	}
+	mutex_unlock(&isert_conn_mutex);
+
+	isert_device_put(found);
+	wait_for_completion(&removal_comp);
+}
+
+static struct ib_client isert_ib_client = {
+	.name   = "isert",
+	.remove = isert_remove_one,
+};
+
 static int __init isert_init(void)
 {
+	int ret = -ENOMEM;
+
 	isert_login_wq = alloc_workqueue("isert_login_wq", WQ_PERCPU, 0);
 	if (!isert_login_wq) {
 		isert_err("Unable to allocate isert_login_wq\n");
@@ -2711,21 +2775,30 @@ static int __init isert_init(void)
 		goto destroy_comp_wq;
 	}
 
+	ret = ib_register_client(&isert_ib_client);
+	if (ret) {
+		isert_err("Unable to register isert ib client\n");
+		goto destroy_release_wq;
+	}
+
 	iscsit_register_transport(&iser_target_transport);
 	isert_info("iSER_TARGET[0] - Loaded iser_target_transport\n");
 
 	return 0;
 
+destroy_release_wq:
+	destroy_workqueue(isert_release_wq);
 destroy_comp_wq:
 	destroy_workqueue(isert_comp_wq);
 destroy_login_wq:
 	destroy_workqueue(isert_login_wq);
 
-	return -ENOMEM;
+	return ret;
 }
 
 static void __exit isert_exit(void)
 {
+	ib_unregister_client(&isert_ib_client);
 	flush_workqueue(isert_login_wq);
 	destroy_workqueue(isert_release_wq);
 	destroy_workqueue(isert_comp_wq);
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 519b17e54bd34..41023300be21b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -191,8 +191,7 @@ struct isert_conn {
 	atomic_t		ctrl_comp_cnt;
 	bool                    logout_posted;
 	bool                    snd_w_inv;
-	wait_queue_head_t	rem_wait;
-	bool			dev_removed;
+	struct list_head	conn_list;
 };
 
 struct isert_device {
@@ -203,6 +202,7 @@ struct isert_device {
 	struct isert_comp	*comps;
 	int                     comps_used;
 	struct list_head	dev_node;
+	struct completion	*removal_comp;
 };
 
 struct isert_np {

Re: [PATCH v2 RESEND] IB/isert: tear connections down from an ib_client on device removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_remove_one() and isert_free_np() on unbound conns
- [Critical] IB/isert: race between isert_connected_handler() and isert_remove_one() causes UAF
- [High] IB/isert: deadlock in isert_remove_one() on ISER_CONN_INIT connections
- [High] IB/isert: deadlock in device removal on racing or init-state connections
--

commit 45ee38abdcac16c81f48c4efea555fb873360442
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: tear connections down from an ib_client on device removal

This commit registers an ib_client for ib_isert to tear down connections and
wait for the last isert_device reference during device removal instead of
waiting in the RDMA_CM_EVENT_DEVICE_REMOVAL callback. It also removes
isert_conn->dev_removed and isert_conn->rem_wait so that
isert_connect_release() is the sole place that destroys the cm_id and frees
isert_conn.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..35e4b9c917bd6 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -625,13 +637,10 @@ isert_np_cma_handler(struct isert_np *isert_np,
>  	return -1;
>  }
>  
> -static int
> -isert_disconnected_handler(struct rdma_cm_id *cma_id,
> -			   enum rdma_cm_event_type event)
> +/* Must be called with isert_conn->mutex held. */
> +static void
> +isert_teardown_conn(struct isert_conn *isert_conn)
>  {
> -	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> -
> -	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
>  		break;
> @@ -648,6 +657,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
>  		isert_warn("conn %p terminating in state %d\n",
>  			   isert_conn, isert_conn->state);
>  	}
> +}

[Severity: High]
What happens in isert_teardown_conn() when a connection is still in
ISER_CONN_INIT during device removal?

During IB device unregistration, disable_device() invokes ib_client->remove
callbacks in reverse registration order:

drivers/infiniband/core/device.c:disable_device() {
	...
	while (cid) {
		cid--;
		remove_client_context(device, cid);
	}
}

Because isert_ib_client is registered after cma_client and cm_client,
isert_remove_one() runs and blocks on wait_for_completion(&removal_comp)
before cma_remove_one() and cm_remove_one() are called.

If a connection has processed RDMA_CM_EVENT_CONNECT_REQUEST in
isert_connect_request(), it holds a reference on isert_device and is on
isert_conn_list in state ISER_CONN_INIT. When isert_remove_one() calls
isert_teardown_conn(), ISER_CONN_INIT hits the default branch and only logs a
warning without terminating the connection.

Because cma_remove_one() and cm_remove_one() have not run yet, the CM remains
active while isert_remove_one() waits on removal_comp. If the peer's RTU
arrives, RDMA_CM_EVENT_ESTABLISHED invokes isert_connected_handler():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn->state = ISER_CONN_UP;
	kref_get(&isert_conn->kref);
	mutex_unlock(&isert_conn->mutex);
}

Since isert_remove_one() already finished its single pass over
isert_conn_list and cma_remove_one() is blocked behind isert_remove_one(),
won't this connection remain in ISER_CONN_UP (or advance to ISER_CONN_BOUND /
ISER_CONN_FULL_FEATURE via isert_accept_np()) without anything ever tearing
it down, causing wait_for_completion(&removal_comp) in isert_remove_one() to
hang forever?

Similarly, on iWARP (where a failed passive connection in ISER_CONN_INIT
delivers IW_CM_EVENT_CLOSE -> RDMA_CM_EVENT_DISCONNECTED rather than
RDMA_CM_EVENT_UNREACHABLE) or if RDMA_CM_EVENT_DEVICE_REMOVAL reaches an
ISER_CONN_INIT connection (where cma_send_device_removal_put() sets
id_priv->state = RDMA_CM_DEVICE_REMOVAL so cma_ib_handler() drops subsequent
IB_CM_REP_ERROR / RDMA_CM_EVENT_UNREACHABLE events), won't
isert_disconnected_handler() -> isert_teardown_conn() ignore ISER_CONN_INIT
and leak the connection reference?

[ ... ]

> @@ -2689,8 +2701,60 @@ static struct iscsit_transport iser_target_transport = {
>  	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
>  };
>  
> +static void
> +isert_remove_one(struct ib_device *ib_device, void *client_data)
> +{
> +	DECLARE_COMPLETION_ONSTACK(removal_comp);
> +	struct isert_device *device, *found = NULL;
> +	struct isert_conn *isert_conn;
> +
> +	/*
> +	 * Not from the RDMA_CM_EVENT_DEVICE_REMOVAL callback: that one runs
> +	 * with the cm_id's handler mutex held, which a teardown already
> +	 * destroying that cm_id needs.  Same reason as
> +	 * commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not
> +	 * deadlock in device removal").
> +	 */
> +	mutex_lock(&device_list_mutex);
> +	list_for_each_entry(device, &device_list, dev_node) {
> +		if (device->ib_device != ib_device)
> +			continue;
> +		device->refcount++;
> +		device->removal_comp = &removal_comp;
> +		found = device;
> +		break;
> +	}
> +	mutex_unlock(&device_list_mutex);
> +
> +	if (!found)
> +		return;

[Severity: High]
Because isert_remove_one() runs before cma_remove_one(), listening RDMA CM
IDs (isert_np->cm_id and per-device child listeners) are still in
RDMA_CM_LISTEN and continue delivering RDMA_CM_EVENT_CONNECT_REQUEST until
cma_remove_one() destroys them.

In isert_connect_request(), isert_device_get() increments device->refcount
under device_list_mutex without checking whether removal is in progress, and
isert_conn is only added to isert_conn_list after allocating login buffers,
creating the QP/CQ, posting a recv WR, and calling isert_rdma_accept():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_request() {
	...
	device = isert_device_get(cma_id);
	if (IS_ERR(device)) {
		ret = PTR_ERR(device);
		goto out;
	}
	isert_conn->device = device;
	...
	mutex_lock(&isert_conn_mutex);
	list_add_tail(&isert_conn->conn_list, &isert_conn_list);
	mutex_unlock(&isert_conn_mutex);
}

If isert_remove_one() scans isert_conn_list while isert_connect_request() is
between isert_device_get() and list_add_tail(&isert_conn->conn_list,
&isert_conn_list), or if a new RDMA_CM_EVENT_CONNECT_REQUEST arrives while
isert_remove_one() is blocked in wait_for_completion(&removal_comp), can the
new connection acquire a reference on found and be added to isert_conn_list
after isert_remove_one() has already finished iterating the list, causing
wait_for_completion(&removal_comp) to hang?

Also, if isert_remove_one() returns (either immediately when found is NULL or
after removal_comp completes) and a new RDMA_CM_EVENT_CONNECT_REQUEST arrives
before cma_remove_one() destroys the listener, won't isert_device_get()
allocate a new struct isert_device on the removing ib_device after
isert_remove_one() has already finished? When cma_remove_one() ->
cma_process_remove() then runs cma_send_device_removal_put() on that new
connection's cm_id, id_priv->state is set to RDMA_CM_DEVICE_REMOVAL and
isert_cma_handler() returns 0; if the connection is still in ISER_CONN_INIT,
cma_ib_handler() drops all subsequent non-timewait IB CM events:

drivers/infiniband/core/cma.c:cma_ib_handler() {
	...
	mutex_lock(&id_priv->handler_mutex);
	state = READ_ONCE(id_priv->state);
	if ((ib_event->event != IB_CM_TIMEWAIT_EXIT &&
	     state != RDMA_CM_CONNECT) ||
	    (ib_event->event == IB_CM_TIMEWAIT_EXIT &&
	     state != RDMA_CM_DISCONNECT))
		goto out;
}

Wouldn't that prevent rdma_destroy_id() from ever being called and deadlock
cma_process_remove() at wait_for_completion(&cma_dev->comp)?

> +
> +	mutex_lock(&isert_conn_mutex);
> +	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> +		if (isert_conn->device != found)
> +			continue;
> +
> +		isert_info("removing conn %p state %d\n",
> +			   isert_conn, isert_conn->state);
> +		mutex_lock(&isert_conn->mutex);
> +		isert_teardown_conn(isert_conn);
> +		mutex_unlock(&isert_conn->mutex);

[Severity: Critical]
Can isert_remove_one() race with isert_free_np() on unbound connections in
ISER_CONN_UP and corrupt drop_conn_list or double-free isert_conn?

When an iSCSI network portal is deleted, isert_free_np() moves connections
from isert_np->pending and isert_np->accepted onto a stack-local
drop_conn_list under isert_np->mutex, and then releases isert_np->mutex
before calling isert_connect_release() on each entry:

drivers/infiniband/ulp/isert/ib_isert.c:isert_free_np() {
	...
	mutex_lock(&isert_np->mutex);
	if (!list_empty(&isert_np->pending)) {
		...
		list_for_each_entry_safe(isert_conn, n,
					 &isert_np->pending,
					 node) {
			...
			list_move_tail(&isert_conn->node, &drop_conn_list);
		}
	}
	...
	mutex_unlock(&isert_np->mutex);

	list_for_each_entry_safe(isert_conn, n, &drop_conn_list, node) {
		list_del_init(&isert_conn->node);
		isert_connect_release(isert_conn);
	}
}

Because those connections remain on isert_conn_list until
isert_connect_release() acquires isert_conn_mutex and removes them, a
concurrent isert_remove_one() walking isert_conn_list can find a connection in
ISER_CONN_UP that isert_free_np() has already moved onto drop_conn_list.

When isert_remove_one() calls isert_teardown_conn() ->
isert_handle_unbound_conn(), it checks !list_empty(&isert_conn->node) under
isert_np->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_handle_unbound_conn() {
	...
	mutex_lock(&isert_np->mutex);
	if (!list_empty(&isert_conn->node)) {
		list_del_init(&isert_conn->node);
		isert_put_conn(isert_conn);
		queue_work(isert_release_wq, &isert_conn->release_work);
	}
	mutex_unlock(&isert_np->mutex);
}

Since isert_conn->node is still linked into drop_conn_list,
!list_empty(&isert_conn->node) is true. Doesn't isert_handle_unbound_conn()
then mutate drop_conn_list via list_del_init(&isert_conn->node) while
isert_free_np() is traversing it without isert_np->mutex, and queue
isert_conn->release_work so that both isert_free_np() and
isert_release_work() call isert_connect_release(isert_conn)?

[Severity: Critical]
Could calling isert_teardown_conn() from isert_remove_one() without the
cm_id's handler_mutex also race with isert_connected_handler() and cause a
use-after-free?

Previously, both isert_connected_handler() and isert_disconnected_handler()
were called from isert_cma_handler() under the cm_id's handler_mutex. In
isert_connected_handler(), isert_conn->mutex is released after setting
ISER_CONN_UP and before acquiring isert_np->mutex to move isert_conn->node
from isert_np->accepted to isert_np->pending:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn->state = ISER_CONN_UP;
	kref_get(&isert_conn->kref);
	mutex_unlock(&isert_conn->mutex);

	mutex_lock(&isert_np->mutex);
	list_move_tail(&isert_conn->node, &isert_np->pending);
	mutex_unlock(&isert_np->mutex);

	isert_info("np %p: Allow accept_np to continue\n", isert_np);
	up(&isert_np->sem);
}

If isert_remove_one() acquires isert_conn->mutex in that window between
mutex_unlock(&isert_conn->mutex) and mutex_lock(&isert_np->mutex),
isert_teardown_conn() sees state == ISER_CONN_UP, sets state to
ISER_CONN_TERMINATING, and calls isert_handle_unbound_conn(isert_conn).
Because isert_conn->node is still on isert_np->accepted,
isert_handle_unbound_conn() removes isert_conn->node with list_del_init(),
drops a kref via isert_put_conn(), and queues isert_conn->release_work.

When isert_connected_handler() then acquires isert_np->mutex, won't
list_move_tail(&isert_conn->node, &isert_np->pending) re-insert the
terminating connection onto isert_np->pending and wake isert_np->sem while
isert_release_work() drops the last kref and frees isert_conn in
isert_connect_release()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060505.409476-1-yhlee@isslab.korea.ac.kr?part=1