Re: [PATCH v4 2/3] IB/isert: drain the QP without holding isert_conn->mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] IB/isert: race with isert_accept_np() when draining QP unlocked
- [Low] IB/isert: missing colon in Return section of isert_conn_terminate() kernel-doc
--

commit 563514f12bfc26c4edaed45f85db0c8c5f4ade31
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: drain the QP without holding isert_conn->mutex

This commit changes isert_conn_terminate() to return whether it moved the
connection to ISER_CONN_TERMINATING, and moves ib_drain_qp() and
isert_handle_unbound_conn() outside isert_conn->mutex in
isert_disconnected_handler() to avoid deadlocking with
isert_login_recv_done().

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 8255208e30c7d..c6f417bb20690 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -578,21 +578,17 @@ isert_handle_unbound_conn(struct isert_conn *isert_conn)
>   * isert_conn_terminate() - Initiate connection termination
>   * @isert_conn: isert connection struct
>   *
> - * Notes:
> - * In case the connection state is BOUND, move state
> - * to TEMINATING and start teardown sequence (rdma_disconnect).
> - * In case the connection state is UP, complete flush as well.
> - *
> - * This routine must be called with mutex held. Thus it is
> - * safe to call multiple times.
> + * Returns true if this caller moved the connection to ISER_CONN_TERMINATING.
>   */

[Severity: Low]
This isn't a bug, but kernel-doc requires a colon after Returns (Returns:)
to recognize the return value section when building with W=2.

> -static void
> +static bool
>  isert_conn_terminate(struct isert_conn *isert_conn)
>  {
>  	int err;
>  
> +	lockdep_assert_held(&isert_conn->mutex);
> +
>  	if (isert_conn->state >= ISER_CONN_TERMINATING)
> -		return;
> +		return false;
>  
>  	isert_info("Terminating conn %p state %d\n",
>  		   isert_conn, isert_conn->state);
> @@ -601,6 +597,8 @@ isert_conn_terminate(struct isert_conn *isert_conn)
>  	if (err)
>  		isert_warn("Failed rdma_disconnect isert_conn %p\n",
>  			   isert_conn);
> +
> +	return true;
>  }
>  
>  static int
> @@ -635,15 +633,14 @@ 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;
> +	bool terminated = false;
>  
>  	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
>  		break;
>  	case ISER_CONN_UP:
> -		isert_conn_terminate(isert_conn);
> -		ib_drain_qp(isert_conn->qp);
> -		isert_handle_unbound_conn(isert_conn);
> +		terminated = isert_conn_terminate(isert_conn);
>  		break;
>  	case ISER_CONN_BOUND:
>  	case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
> @@ -655,6 +652,11 @@ 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);
> +	}

[Severity: High]
Can dropping isert_conn->mutex before ib_drain_qp() and
isert_handle_unbound_conn() race with isert_accept_np()?

When a connection reaches isert_connected_handler(), its state is set to
ISER_CONN_UP, isert_conn->node is placed on isert_np->pending, and
up(&isert_np->sem) wakes the login thread in isert_accept_np().

If a disconnect arrives while the connection is still in ISER_CONN_UP,
isert_disconnected_handler() sets isert_conn->state to
ISER_CONN_TERMINATING and now unlocks isert_conn->mutex before removing
isert_conn->node from isert_np->pending in isert_handle_unbound_conn().

While ib_drain_qp() is waiting in isert_disconnected_handler(),
isert_login_recv_done() can complete isert_conn->login_req_comp for a login
PDU received just before the disconnect, and isert_accept_np() can dequeue
isert_conn from isert_np->pending and overwrite isert_conn->state without
holding isert_conn->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_accept_np() {
	...
	isert_conn = list_first_entry(&isert_np->pending,
			struct isert_conn, node);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);

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

Once list_del_init(&isert_conn->node) has run in isert_accept_np(),
isert_handle_unbound_conn() sees list_empty(&isert_conn->node) and becomes
a no-op.

If login then fails or times out, iscsi_target_login_sess_out() calls
isert_wait_conn() and isert_free_conn():

drivers/infiniband/ulp/isert/ib_isert.c:isert_wait_conn() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn_terminate(isert_conn);
	mutex_unlock(&isert_conn->mutex);

	ib_drain_qp(isert_conn->qp);
	isert_put_conn(isert_conn);
	queue_work(isert_release_wq, &isert_conn->release_work);
}

Because isert_conn->state was reset to ISER_CONN_BOUND and
isert_conn->mutex is no longer held across ib_drain_qp(), wouldn't
isert_wait_conn() call ib_drain_qp(isert_conn->qp) a second time
concurrently with isert_disconnected_handler(), and allow both kref
references on isert_conn to be dropped while isert_disconnected_handler()
is still running?

Alternatively, if single-stage login succeeds in memory without posting to
the QP (isert_put_login_tx() sets isert_conn->login_rsp_pending = true),
wouldn't isert_get_rx_pdu() set isert_conn->state to ISER_CONN_FULL_FEATURE,
post to the already-disconnected QP, and block indefinitely in
wait_for_completion_interruptible(&comp) because the disconnect event was
already handled?

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