Re: [PATCH v3 2/2] 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: UAF and invalid reject on removing check in isert_connect_request()
- [High] IB/isert: hang and connection leak in isert_teardown_conn() on ISER_CONN_INIT
- [High] IB/isert: new isert_device and conn created after isert_remove_one() runs
- [High] IB/isert: ABBA deadlock calling ib_drain_qp() under isert_conn->mutex
--

commit 0865225bbb6b53b508af1b272b6ebb49c8aac4d5
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

This commit replaces the RDMA_CM_EVENT_DEVICE_REMOVAL wait and kfree with an
ib_client removal callback (isert_remove_one) that tears down active
connections and waits for the last isert_device reference to be released.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 5371bba71ba3c..223a727c1d0de 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
[ ... ]
> @@ -464,9 +470,19 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>  	if (ret)
>  		goto out_destroy_qp;
>  
> +	mutex_lock(&isert_conn_mutex);
> +	if (device->removing) {
> +		mutex_unlock(&isert_conn_mutex);
> +		isert_info("device %p is being removed, rejecting conn %p\n",
> +			   device, isert_conn);
> +		ret = -ENODEV;
> +		goto out_destroy_qp;
> +	}

[Severity: Critical]
By the time isert_connect_request() acquires isert_conn_mutex and checks
device->removing, it has already called isert_login_post_recv() and
isert_rdma_accept():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_request() {
	...
	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;

	mutex_lock(&isert_conn_mutex);
	if (device->removing) {
		...
		ret = -ENODEV;
		goto out_destroy_qp;
	}
	...
out_destroy_qp:
	isert_destroy_qp(isert_conn);
out_rsp_dma_map:
	isert_free_login_buf(isert_conn);
out_conn_dev:
	isert_device_put(device);
out:
	kfree(isert_conn);
	rdma_reject(cma_id, NULL, 0, IB_CM_REJ_CONSUMER_DEFINED);
	return ret;
}

Because rdma_accept() has already transitioned isert_conn->qp to RTR/RTS,
jumping to out_destroy_qp destroys the QP and returns the shared
IB_POLL_WORKQUEUE CQ to the CQ pool via ib_cq_pool_put() without calling
ib_drain_qp(), then immediately frees login_desc, device, and isert_conn. If a
receive or flush CQE for the posted login_desc->rx_cqe was already queued on
the shared CQ, won't ib_cq_poll_work() invoke isert_login_recv_done() on the
freed login_desc, isert_conn, and device?

In addition, jumping to out after rdma_accept() succeeded calls rdma_reject()
and returns -ENODEV to the CMA REQ handler on an already-accepted cm_id. On
iWARP, iw_cm_accept() has already set IWCM_F_CONNECT_WAIT and taken a
reference on isert_conn->qp, which causes iw_destroy_cm_id() to hang waiting
for IWCM_F_CONNECT_WAIT to clear. Should the device->removing check happen
before isert_login_post_recv() and isert_rdma_accept() while holding a lock
that serializes against isert_remove_one()?

[ ... ]
> @@ -521,13 +537,25 @@ isert_connected_handler(struct rdma_cm_id *cma_id)
>  	isert_info("conn %p\n", isert_conn);
>  
>  	mutex_lock(&isert_conn->mutex);
> +	if (isert_conn->state >= ISER_CONN_TERMINATING) {
> +		/* The login receive is still posted, drain before releasing. */
> +		isert_info("conn %p is terminating, state %d\n",
> +			   isert_conn, isert_conn->state);
> +		if (rdma_disconnect(isert_conn->cm_id))
> +			isert_warn("Failed rdma_disconnect isert_conn %p\n",
> +				   isert_conn);
> +		ib_drain_qp(isert_conn->qp);
> +		isert_handle_unbound_conn(isert_conn, false);
> +		mutex_unlock(&isert_conn->mutex);
> +		return;
> +	}

[Severity: High]
Can calling ib_drain_qp(isert_conn->qp) while holding isert_conn->mutex here
(and in isert_teardown_conn() under the ISER_CONN_UP case) cause an ABBA
deadlock with the CQ completion workqueue?

In isert_create_qp(), isert_conn->cq is allocated from the shared pool with
IB_POLL_WORKQUEUE. If the initiator's login request PDU arrives before
rdma_disconnect() moves the QP to error, login_desc->rx_cqe completes with
IB_WC_SUCCESS ahead of the drain CQE on isert_conn->cq, and ib_cq_poll_work()
runs isert_login_recv_done():

drivers/infiniband/ulp/isert/ib_isert.c:isert_login_recv_done() {
	...
	mutex_lock(&isert_conn->mutex);
	complete(&isert_conn->login_req_comp);
	mutex_unlock(&isert_conn->mutex);
	...
}

Because isert_login_recv_done() blocks on isert_conn->mutex, ib_cq_poll_work()
cannot advance to the drain CQE that ib_drain_qp() is waiting on while holding
isert_conn->mutex (and isert_conn_mutex when called from isert_remove_one()).

[ ... ]
> @@ -625,20 +654,23 @@ 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;
> +	lockdep_assert_held(&isert_conn->mutex);
>  
> -	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
> +	case ISER_CONN_DOWN:
> +		break;
> +	case ISER_CONN_INIT:
> +		/* Mark only; isert_connected_handler() releases it. */
> +		isert_conn->state = ISER_CONN_TERMINATING;
>  		break;

[Severity: High]
As reported by Sashiko on v2 [1], when a connection in ISER_CONN_INIT is torn
down in isert_teardown_conn(), it is only marked ISER_CONN_TERMINATING and
relies on isert_connected_handler() (RDMA_CM_EVENT_ESTABLISHED) to disconnect,
drain the QP, and schedule release_work. What happens if
RDMA_CM_EVENT_ESTABLISHED never arrives?

For example, if the peer disconnects after CM REQ/REP without sending RTU (or
closes the connection before establishment on iWARP), cma_ib_handler() /
cma_iw_handler() delivers RDMA_CM_EVENT_DISCONNECTED, which calls
isert_disconnected_handler() -> isert_teardown_conn(). That transitions
isert_conn->state to ISER_CONN_TERMINATING without scheduling cleanup, and
since the CM state is now disconnecting, neither RDMA_CM_EVENT_ESTABLISHED nor
RDMA_CM_EVENT_UNREACHABLE will ever fire. Similarly, if isert_remove_one()
runs while a connection is in ISER_CONN_INIT and the peer never sends RTU,
isert_remove_one() runs before cma_remove_one() and cm_remove_one() in the
ib_client removal order. Won't that leak isert_conn and its isert_device
reference and cause isert_remove_one() to hang indefinitely in
wait_for_completion(&removal_comp)?

[ ... ]
> @@ -2697,8 +2727,54 @@ 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;
> +
> +	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);
> +	found->removing = true;
> +	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);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], during IB device unregistration,
disable_device() invokes ib_client remove callbacks in reverse registration
order, so isert_remove_one() runs and returns before cma_remove_one() destroys
listening rdma_cm_ids.

Because struct isert_device entries only exist on device_list while
device->refcount > 0, if no connections exist when isert_remove_one() runs
(found == NULL), or after wait_for_completion(&removal_comp) returns and found
has been removed from device_list and freed by isert_device_put(), there is no
record that ib_device is being removed.

If an RDMA_CM_EVENT_CONNECT_REQUEST arrives on a still-active listener before
cma_remove_one() runs, isert_connect_request() -> isert_device_get() will
allocate a fresh struct isert_device with removing == false and accept the new
connection in ISER_CONN_INIT after isert_remove_one() has already finished.
When cma_remove_one() -> cma_process_remove() subsequently delivers
RDMA_CM_EVENT_DEVICE_REMOVAL to that connection, isert_cma_handler() returns 0
and isert_teardown_conn() only sets state = ISER_CONN_TERMINATING while
id_priv->state is set to RDMA_CM_DEVICE_REMOVAL (suppressing any further CM
events). Won't cma_process_remove() then block forever waiting for that cm_id
to be destroyed?

[1] https://lore.kernel.org/r/20261026060505.409476-1-yhlee@isslab.korea.ac.kr
[2] https://lore.kernel.org/r/20261026060505.409476-1-yhlee@isslab.korea.ac.kr

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