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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_teardown_conn() and isert_connect_error()
- [High] IB/isert: QP destroyed after accept in isert_connect_request() on removal
- [High] IB/isert: new isert_device created after isert_remove_one() completes
- [High] IB/isert: race between isert_teardown_conn() and isert_accept_np()
--

commit b4ccbe89413aac4cc5fc070bd4b49d33beb67439
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 removal callback to tear down iSER
connections without holding the rdma_cm_id handler_mutex, waiting for the
last isert_device reference to be dropped and reference counting isert_np.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index c6f417bb20690..5486fbdc6aa39 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -456,20 +466,42 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  		goto out_rsp_dma_map;
>  	}
>  
> +	if (READ_ONCE(device->removing)) {
> +		isert_info("device %p is being removed, rejecting conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_destroy_qp;
> +	}
> +
>  	ret = isert_login_post_recv(isert_conn);
>  	if (ret)
>  		goto out_destroy_qp;
>  
>  	ret = isert_rdma_accept(isert_conn);
>  	if (ret)
> -		goto out_destroy_qp;
> -
> +		goto out_drain_qp;
> +	accepted = true;
> +
> +	mutex_lock(&isert_conn_mutex);
> +	if (device->removing) {
> +		mutex_unlock(&isert_conn_mutex);
> +		isert_info("device %p is being removed, dropping conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_drain_qp;
> +	}
> +	isert_conn->np = isert_np;
> +	kref_get(&isert_np->kref);
> +	list_add_tail(&isert_conn->conn_list, &isert_conn_list);
>  	mutex_lock(&isert_np->mutex);
>  	list_add_tail(&isert_conn->node, &isert_np->accepted);
>  	mutex_unlock(&isert_np->mutex);
> +	mutex_unlock(&isert_conn_mutex);
>  
>  	return 0;
>  
> +out_drain_qp:
> +	ib_drain_qp(isert_conn->qp);
>  out_destroy_qp:
>  	isert_destroy_qp(isert_conn);
>  out_rsp_dma_map:
> @@ -478,7 +510,9 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  	isert_device_put(device);
>  out:
>  	kfree(isert_conn);
> -	rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
> +	/* cma destroys an accepted cm id when this handler returns an error. */
> +	if (!accepted)
> +		rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
>  	return ret;
>  }

[Severity: High]
As reported by Sashiko on v3 [1], if device->removing becomes true between
the unlocked READ_ONCE(device->removing) check and mutex_lock(&isert_conn_mutex)
in isert_connect_request(), isert_rdma_accept() has already succeeded, yet
the error path jumps to out_drain_qp -> out_destroy_qp and calls
ib_destroy_qp(isert_conn->qp) before returning -ENODEV to the RDMA CM
CONNECT_REQUEST handler.

On iWARP transports, cma_accept_iw() -> iw_cm_accept() sets
IWCM_F_CONNECT_WAIT, stores cm_id_priv->qp = qp, and takes a provider QP
reference via iw_add_ref(qp) that is not released until iw_destroy_cm_id() or
cm_close_handler() runs on iwcm_wq after the CONNECT_REQUEST callback
returns. Won't calling ib_destroy_qp() here before returning from
isert_connect_request() deadlock in iWARP provider destroy_qp callbacks that
wait for the QP reference count to drop to zero (such as siw_destroy_qp() or
_irdma_destroy_qp()), or deadlock destroy_cm_id() on iwcm_wq waiting for
IWCM_F_CONNECT_WAIT to clear after iw_conn_req_handler() returns an error?

Also on IB/RoCE, doesn't destroying isert_conn->qp directly with
ib_destroy_qp() after cma_accept_ib() has already sent the CM REP leave
cma_id->qp pointing to a freed QP while the CM ID is still being torn down?

[ ... ]

> @@ -628,17 +695,17 @@ 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)
> +static void
> +isert_teardown_conn(struct isert_conn *isert_conn)
>  {
> -	struct isert_conn *isert_conn = cma_id->qp->qp_context;
>  	bool terminated = false;
>  
>  	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
> +	case ISER_CONN_DOWN:
>  		break;
> +	case ISER_CONN_INIT:
>  	case ISER_CONN_UP:
>  		terminated = isert_conn_terminate(isert_conn);
>  		break;
> @@ -652,10 +719,20 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
>  	}
>  	mutex_unlock(&isert_conn->mutex);
>  
> -	if (terminated) {
> -		ib_drain_qp(isert_conn->qp);
> -		isert_handle_unbound_conn(isert_conn);
> -	}
> +	if (!terminated)
> +		return;
> +
> +	ib_drain_qp(isert_conn->qp);
> +	isert_handle_unbound_conn(isert_conn);
> +}

[Severity: Critical]
Can isert_teardown_conn() called from isert_remove_one() race with
isert_connect_error()?

Previously, connection teardown and connection error handling both ran under
the cm_id handler_mutex, and ISER_CONN_INIT was not handled in
isert_disconnected_handler(). With this change, isert_remove_one() takes a
reference on isert_conn via kref_get_unless_zero(&isert_conn->kref) and calls
isert_teardown_conn() outside the cm_id handler_mutex, while
isert_connect_error() does not take isert_conn->mutex or update
isert_conn->state:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_error() {
	mutex_lock(&isert_np->mutex);
	owned = !list_empty(&isert_conn->node);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);

	if (!owned) {
		isert_info("conn %p is being released already\n", isert_conn);
		return 0;
	}

	ib_drain_qp(isert_conn->qp);
	isert_conn->cm_id = NULL;
	isert_put_conn(isert_conn);

	return -1;
}

If isert_connect_error() runs after isert_remove_one() grabs a kref on a
connection in ISER_CONN_INIT and before isert_remove_one() calls
isert_teardown_conn(), isert_connect_error() sets isert_conn->cm_id = NULL,
drops only the initial kref, and returns -1 (causing RDMA CM to destroy
cma_id), leaving isert_conn->state as ISER_CONN_INIT. When
isert_remove_one() then calls isert_teardown_conn() ->
isert_conn_terminate(), won't rdma_disconnect(isert_conn->cm_id) dereference
a NULL (or freed) cm_id?

Conversely, if isert_teardown_conn() runs isert_conn_terminate() first, it
sets isert_conn->state = ISER_CONN_TERMINATING and drops isert_conn->mutex to
call ib_drain_qp(isert_conn->qp) before isert_handle_unbound_conn() unlinks
isert_conn->node. While ib_drain_qp() is sleeping, can't a concurrent
isert_connect_error() see !list_empty(&isert_conn->node), unlink it, call
ib_drain_qp(isert_conn->qp) concurrently on the same QP, clear
isert_conn->cm_id = NULL, and free cma_id?

Also, if RDMA_CM_EVENT_UNREACHABLE triggers isert_connect_error() after
isert_connected_handler() has promoted the connection to ISER_CONN_UP (with
isert_conn->promoted == true and an extra kref), isert_connect_error()
unlinks isert_conn->node without dropping the promoted reference. Won't that
leak isert_conn and hang wait_for_completion(&removal_comp) in
isert_remove_one()?

[ ... ]

> @@ -2480,6 +2546,7 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
>  	isert_conn = list_first_entry(&isert_np->pending,
>  			struct isert_conn, node);
>  	list_del_init(&isert_conn->node);
> +	isert_conn->promoted = false;
>  	mutex_unlock(&isert_np->mutex);
>  
>  	conn->context = isert_conn;

[Severity: High]
What happens if isert_teardown_conn() races with isert_accept_np() on a
connection in ISER_CONN_UP?

In isert_teardown_conn(), when isert_conn->state is ISER_CONN_UP, it sets
isert_conn->state = ISER_CONN_TERMINATING under isert_conn->mutex, unlocks
isert_conn->mutex, and calls ib_drain_qp(isert_conn->qp) before calling
isert_handle_unbound_conn() to unlink isert_conn->node from
isert_np->pending.

During that ib_drain_qp() window, isert_accept_np() only acquires
isert_np->mutex (without acquiring isert_conn->mutex or checking
isert_conn->state):

	mutex_lock(&isert_np->mutex);
	if (list_empty(&isert_np->pending)) {
		mutex_unlock(&isert_np->mutex);
		goto accept_wait;
	}
	isert_conn = list_first_entry(&isert_np->pending,
			struct isert_conn, node);
	list_del_init(&isert_conn->node);
	isert_conn->promoted = false;
	mutex_unlock(&isert_np->mutex);

	conn->context = isert_conn;
	isert_conn->conn = conn;
	isert_conn->state = ISER_CONN_BOUND;

If isert_accept_np() pops the connection from isert_np->pending while
ib_drain_qp() is in progress, it overwrites isert_conn->state from
ISER_CONN_TERMINATING to ISER_CONN_BOUND. Then isert_handle_unbound_conn()
sees list_empty(&isert_conn->node) and does nothing, while
isert_teardown_conn() never called iscsit_cause_connection_reinstatement()
because the state was ISER_CONN_UP when it checked. If the login request PDU
already arrived before the QP drained and finishes a single-stage login, the
login timer is stopped and iscsi_target_rx_thread() blocks in
isert_get_rx_pdu() -> wait_for_completion_interruptible(&comp) on the
already-drained QP. Doesn't this leave the connection stuck and hang
isert_remove_one() at wait_for_completion(&removal_comp)?

[ ... ]

> @@ -2717,8 +2783,61 @@ 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, *tmp;
> +	LIST_HEAD(removal_list);
> +
> +	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);
> +	WRITE_ONCE(found->removing, true);
> +	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> +		if (isert_conn->device != found)
> +			continue;
> +		/* A connection with no reference left is already releasing. */
> +		if (!kref_get_unless_zero(&isert_conn->kref))
> +			continue;
> +		list_add_tail(&isert_conn->removal_node, &removal_list);
> +	}
> +	mutex_unlock(&isert_conn_mutex);
> +
> +	list_for_each_entry_safe(isert_conn, tmp, &removal_list, removal_node) {
> +		list_del_init(&isert_conn->removal_node);
> +		isert_info("removing conn %p state %d\n",
> +			   isert_conn, isert_conn->state);
> +		isert_teardown_conn(isert_conn);
> +		isert_put_conn(isert_conn);
> +	}
> +
> +	isert_device_put(found);
> +	wait_for_completion(&removal_comp);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], because isert_ib_client is registered after
cma_client, disable_device() invokes isert_remove_one() before
cma_remove_one() destroys listening RDMA CM IDs.

In ib_isert.c, struct isert_device is only present on device_list while its
refcount is non-zero: if there are no active connections when
isert_remove_one() runs (or once existing connections drop the refcount to 0
in isert_device_put() and complete removal_comp), the isert_device is removed
from device_list and freed, and isert_remove_one() returns.

If an incoming RDMA_CM_EVENT_CONNECT_REQUEST arrives on a listener before
cma_remove_one() runs, won't isert_device_get() find no matching entry in
device_list, allocate a fresh struct isert_device with removing == false, and
accept the new connection after isert_remove_one() has already completed?
When cma_remove_one() then delivers RDMA_CM_EVENT_DEVICE_REMOVAL to that
connection, isert_disconnected_handler() queues release_work asynchronously,
and isert_connect_release() calls rdma_destroy_id() (which unblocks
cma_remove_one()) before destroying the QP, returning the shared CQ to the
pool, unmapping DMA buffers, and deallocating the PD.

[1] https://lore.kernel.org/r/20261102061608.617186-3-yhlee@isslab.korea.ac.kr
[2] https://lore.kernel.org/r/sashiko-outbox-184744@kernel.org

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