Re: [PATCH v5 3/5] 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 1 potential issue(s) to consider:
- [High] IB/isert: premature reinstatement in isert_teardown_conn() hangs login
--

commit fa784d352b38db9ec9f326e2a339d1ae4eb8f372
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 to tear down iSER target connections
during RDMA device removal instead of waiting for connection release inside
the RDMA_CM_EVENT_DEVICE_REMOVAL callback. It also adds reference counting
for isert_np and tracks active connections on isert_conn_list.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 61b214012a70b..4812dabd93945 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
[ ... ]
> @@ -610,48 +668,50 @@ 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;
> -	struct isert_np *isert_np = cma_id->context;
> -	bool owned = false, bound = false, terminated = false;
> +	struct isert_np *isert_np = isert_conn->np;
> +	bool owned = false, bound = false;
> +	bool promoted = false, terminated = false;
>  
>  	mutex_lock(&isert_conn->mutex);
> -	switch (isert_conn->state) {
> -	case ISER_CONN_UP:
> -		mutex_lock(&isert_np->mutex);
> -		if (list_empty(&isert_conn->node)) {
> -			bound = isert_conn->state == ISER_CONN_BOUND ||
> -				isert_conn->state == ISER_CONN_FULL_FEATURE;
> -		} else {
> -			owned = true;
> -			list_del_init(&isert_conn->node);
> -		}
> -		mutex_unlock(&isert_np->mutex);
> -		break;
> -	case ISER_CONN_BOUND:
> -	case ISER_CONN_FULL_FEATURE: /* FALLTHRU */
> -		bound = true;
> -		break;
> -	default:
> -		break;
> +	mutex_lock(&isert_np->mutex);
> +	if (list_empty(&isert_conn->node)) {
> +		bound = isert_conn->state == ISER_CONN_BOUND ||
> +			isert_conn->state == ISER_CONN_FULL_FEATURE;
> +	} else {
> +		owned = true;
> +		promoted = isert_conn->promoted;
> +		list_del_init(&isert_conn->node);
>  	}
> +	mutex_unlock(&isert_np->mutex);
>  
> -	if (owned)
> +	if (owned) {
>  		terminated = isert_conn_terminate(isert_conn);
> -	else if (bound)
> +	} else if (bound) {
> +		isert_conn_terminate(isert_conn);
>  		iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
> +	}
>  	mutex_unlock(&isert_conn->mutex);

[ ... ]

> @@ -661,20 +721,33 @@ isert_connect_error(struct rdma_cm_id *cma_id)
>  {
>  	struct isert_conn *isert_conn = cma_id->qp->qp_context;
>  	struct isert_np *isert_np = cma_id->context;
> -	bool owned;
> +	bool owned = false, bound = false;
>  
> +	mutex_lock(&isert_conn->mutex);
>  	mutex_lock(&isert_np->mutex);
> -	owned = !list_empty(&isert_conn->node);
> -	list_del_init(&isert_conn->node);
> +	if (list_empty(&isert_conn->node)) {
> +		bound = isert_conn->state == ISER_CONN_BOUND ||
> +			isert_conn->state == ISER_CONN_FULL_FEATURE;
> +	} else {
> +		owned = true;
> +		list_del_init(&isert_conn->node);
> +	}
>  	mutex_unlock(&isert_np->mutex);
>  
> +	if (owned) {
> +		isert_conn->cm_id = NULL;
> +	} else if (bound) {
> +		isert_conn_terminate(isert_conn);
> +		iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
> +	}
> +	mutex_unlock(&isert_conn->mutex);

[ ... ]

> @@ -2679,6 +2741,10 @@ static void isert_get_rx_pdu(struct iscsit_conn *conn)
>  
>  	/* Now we are in FULL_FEATURE phase */
>  	mutex_lock(&isert_conn->mutex);
> +	if (isert_conn->state >= ISER_CONN_TERMINATING) {
> +		mutex_unlock(&isert_conn->mutex);
> +		return;
> +	}
>  	isert_conn->state = ISER_CONN_FULL_FEATURE;
>  	mutex_unlock(&isert_conn->mutex);

[Severity: High]
Can calling iscsit_cause_connection_reinstatement() in isert_teardown_conn()
and isert_connect_error() while isert_conn->state is ISER_CONN_BOUND leak
isert_conn and hang device removal in isert_remove_one()?

In isert_accept_np(), isert_conn->state is set to ISER_CONN_BOUND and stays
there throughout login negotiation until isert_get_rx_pdu() sets
ISER_CONN_FULL_FEATURE. At the end of login negotiation,
iscsi_target_do_tx_login_io() starts the RX and TX kthreads while
conn->conn_state is still TARG_CONN_STATE_IN_LOGIN and before
iscsi_post_login_handler() completes conn->rx_login_comp:

  iscsi_target_do_login_io()
    iscsi_target_do_tx_login_io()
      iscsit_start_kthreads(conn)
        conn->tx_thread_active = true;
        conn->rx_thread_active = true;
      iscsit_put_login_tx() -> isert_put_login_tx()
        isert_conn->login_rsp_pending = true;

If isert_teardown_conn() or isert_connect_error() runs in this window,
bound is true because isert_conn->state == ISER_CONN_BOUND, so it calls
iscsit_cause_connection_reinstatement(isert_conn->conn, 0), sending SIGINT
to both conn->rx_thread and conn->tx_thread.

In drivers/target/iscsi/iscsi_target.c:iscsi_target_rx_thread(), SIGINT
interrupts wait_for_completion_interruptible(&conn->rx_login_comp) before
iscsit_get_rx_pdu() -> isert_get_rx_pdu() is reached:

	rc = wait_for_completion_interruptible(&conn->rx_login_comp);
	if (rc < 0 || iscsi_target_check_conn_state(conn))
		goto out;

And in drivers/target/iscsi/iscsi_target.c:iscsi_target_tx_thread(), waking
on SIGINT jumps to transport_err, where conn->conn_state is still
TARG_CONN_STATE_IN_LOGIN:

transport_err:
	if (conn->conn_state != TARG_CONN_STATE_IN_LOGIN)
		iscsit_take_action_for_connection_exit(conn, &conn_freed);

Neither thread calls iscsit_take_action_for_connection_exit() to close the
connection and invoke isert_free_conn(), leaving both threads sleeping in
while (!kthread_should_stop()) and causing isert_remove_one() to wait
indefinitely on removal_comp.

Furthermore, if iscsi_target_tx_thread() reaches transport_err just after
iscsi_post_login_handler() sets conn->conn_state = TARG_CONN_STATE_LOGGED_IN
but before it adds conn->conn_list to sess->sess_conn_list,
iscsit_close_connection() will run list_del(&conn->conn_list) on an
uninitialized list head while iscsi_post_login_handler() is still running.

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