Re: [PATCH 2/3] scsi: target: iscsi: don't attach a connection to a closing session

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: target: iscsi: incomplete teardown check in
  iscsi_post_login_handler()
- [Medium] scsi: target: iscsi: premature LOGGED_IN state in
  iscsi_post_login_handler()
--

commit fbb74f9a87b2184ab2a30c73faf991dfc1060e88
Author: Daniel Wade <danjwade95@gmail.com>

scsi: target: iscsi: don't attach a connection to a closing session

This commit sets sess->session_close under sess->conn_lock when the last
connection closes in the ERL=0 fallback path and re-checks
sess->session_close in iscsi_post_login_handler() before attaching a
non-leading connection to the session.

> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index f4df551817116..de0cd95aff017 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -660,6 +660,33 @@ void iscsit_stop_kthreads(struct iscsit_conn *conn)
>  	spin_unlock(&iscsit_global->ts_bitmap_lock);
>  }
>  
> +/*
> + * Drop a connection whose kthreads are already running but that must not enter
> + * full feature phase.  conn->sess must not be touched once this returns.
> + */
> +static void iscsi_post_login_fail_conn(struct iscsit_conn *conn)
> +{
> +	/*
> +	 * iscsi_target_tx_thread() only leaves cleanup to this context while
> +	 * conn_state is TARG_CONN_STATE_IN_LOGIN; otherwise it would reach
> +	 * iscsit_close_connection() and walk the session being refused.
> +	 */
> +	conn->conn_state = TARG_CONN_STATE_IN_LOGIN;
> +	iscsit_stop_kthreads(conn);
> +	iscsit_dec_conn_usage_count(conn);
> +
> +	if (conn->sock) {
> +		sock_release(conn->sock);
> +		conn->sock = NULL;
> +	}
> +	if (conn->conn_transport->iscsit_wait_conn)
> +		conn->conn_transport->iscsit_wait_conn(conn);
> +	if (conn->conn_transport->iscsit_free_conn)
> +		conn->conn_transport->iscsit_free_conn(conn);
> +
> +	iscsit_free_conn(conn);
> +}

[Severity: Medium]
Can setting conn->conn_state = TARG_CONN_STATE_LOGGED_IN at the start of
iscsi_post_login_handler() and only resetting it back to
TARG_CONN_STATE_IN_LOGIN here deadlock with conn->tx_thread?

iscsi_post_login_handler() increments conn->conn_usage_count and sets
conn->conn_state = TARG_CONN_STATE_LOGGED_IN before taking sess->conn_lock
to check sess->session_close:

drivers/target/iscsi/iscsi_target_login.c:iscsi_post_login_handler() {
	iscsit_inc_conn_usage_count(conn);

	iscsit_collect_login_stats(conn, ISCSI_STATUS_CLS_SUCCESS,
			ISCSI_LOGIN_STATUS_ACCEPT);

	pr_debug("Moving to TARG_CONN_STATE_LOGGED_IN.\n");
	conn->conn_state = TARG_CONN_STATE_LOGGED_IN;

	iscsi_set_connection_parameters(conn->conn_ops, conn->param_list);
	/*
	 * SCSI Initiator -> SCSI Target Port Mapping
	 */
	if (!zero_tsih) {
		iscsi_set_session_parameters(sess->sess_ops,
				conn->param_list, 0);
}

If an asynchronous transport disconnect sends SIGINT to conn->tx_thread
during that window (for example via isert_disconnected_handler() ->
iscsit_cause_connection_reinstatement()), tx_thread sees conn_state !=
TARG_CONN_STATE_IN_LOGIN and enters iscsit_close_connection() on the
unattached connection:

CPU0 (login worker)               CPU1 (conn->tx_thread)
iscsi_post_login_handler()
  iscsit_inc_conn_usage_count()
  conn->conn_state =
    TARG_CONN_STATE_LOGGED_IN
                                  iscsi_target_tx_thread()
                                    if (conn_state !=
                                        TARG_CONN_STATE_IN_LOGIN)
                                      iscsit_take_action_...()
                                        iscsit_close_connection()
                                          iscsit_check_conn_usage_count()
                                            wait_for_completion()
  iscsi_post_login_fail_conn()
    conn->conn_state =
      TARG_CONN_STATE_IN_LOGIN
    iscsit_stop_kthreads()
      kthread_stop(tx_thread)  <-- deadlocks waiting for tx_thread
    iscsit_dec_conn_usage_count()

In addition, iscsi_set_session_parameters() mutates sess->sess_ops and
iscsit_collect_login_stats() records an accepted login before the session
is checked under sess->conn_lock.  Checking whether the session is closing
before transitioning conn->conn_state or mutating session state would
avoid this window.

>  void iscsi_post_login_handler(
>  	struct iscsi_np *np,
>  	struct iscsit_conn *conn,
> @@ -690,6 +717,20 @@ void iscsi_post_login_handler(
>  		conn->param_list = NULL;
>  
>  		spin_lock_bh(&sess->conn_lock);
> +		/*
> +		 * The skip filter in iscsi_login_non_zero_tsih_s2() sampled this
> +		 * before the negotiation.  Re-test under the lock that serialises
> +		 * against the ->nconn test in iscsit_close_connection().
> +		 */
> +		if (atomic_read(&sess->session_close)) {
> +			spin_unlock_bh(&sess->conn_lock);
> +			pr_err("iSCSI session is closing, failing login on CID: %u\n",
> +			       conn->cid);
> +			iscsi_post_login_fail_conn(conn);
> +			/* Last access to @sess: this lets close_session() free it. */
> +			iscsit_dec_session_usage_count(sess);
> +			return;
> +		}
>  		atomic_set(&sess->session_continuation, 0);
>  		if (sess->session_state == TARG_SESS_STATE_FAILED) {
>  			pr_debug("Moving to"

[Severity: High]
Does checking only sess->session_close here still allow a non-leading
connection to attach to a session that is already tearing down?

iscsi_login_non_zero_tsih_s2() skips sessions that have any of
session_fall_back_to_erl0, session_logout, session_close or
ISCSI_TF_EXPIRED set, but iscsi_post_login_handler() only re-checks
session_close, and iscsit_close_connection() only sets session_close when
sess->nconn reaches 0 in the ERL=0 fallback branch.

For example, if a session has one active connection (conn1) when conn2
passes iscsi_login_non_zero_tsih_s2(), sess->session_state is
TARG_SESS_STATE_LOGGED_IN, so sess->session_continuation remains 0 while
conn2 negotiates holding session_usage_count.  If conn1 then closes via a
connection logout (or ERL=2 failure) and the Time2Retain timer expires,
iscsit_handle_time2retain_timeout() sets ISCSI_TF_EXPIRED and calls
iscsit_close_session(sess, false) without setting sess->session_close:

CPU0 (conn1 / timer)              CPU1 (conn2 login)
iscsit_close_connection(conn1)
  sess->session_state =
    TARG_SESS_STATE_FAILED
  iscsit_start_time2retain_...()
<softirq>
  iscsit_handle_time2retain_...()
    sess->time2retain_timer_flags |=
      ISCSI_TF_EXPIRED
    iscsit_close_session(sess, false)
                                  iscsi_post_login_handler(conn2)
                                    spin_lock_bh(&sess->conn_lock)
                                    atomic_read(&sess->session_close) == 0
                                    list_add_tail(&conn2->conn_list, ...)
                                    atomic_inc(&sess->nconn)
                                    spin_unlock_bh(&sess->conn_lock)
      if (atomic_read(&sess->nconn))
        BUG()  <-- kernel panic (or kfree(sess) if past the nconn check)

Similarly, in a multi-connection session (nconn == 2) falling back to
ERL=0, when the first connection closes it sets session_fall_back_to_erl0
and session_stop_active under sess->conn_lock without setting
session_close, and calls iscsit_stop_session().  If conn2 attaches in
iscsi_post_login_handler() before the second connection takes
sess->conn_lock in iscsit_close_connection(), the second connection
decrements nconn from 2 to 1, sees session_stop_active is already 1 and
returns early:

drivers/target/iscsi/iscsi_target.c:iscsit_close_connection() {
	if (atomic_read(&sess->nconn)) {
		if (!atomic_read(&sess->session_reinstatement) &&
		    !atomic_read(&sess->session_fall_back_to_erl0)) {
			spin_unlock_bh(&sess->conn_lock);
			return 0;
		}
		if (!atomic_read(&sess->session_stop_active)) {
			atomic_set(&sess->session_stop_active, 1);
			spin_unlock_bh(&sess->conn_lock);
			iscsit_stop_session(sess, 0, 0);
			return 0;
		}
		spin_unlock_bh(&sess->conn_lock);
		return 0;
	}
}

That leaves conn2 stranded on a session with session_fall_back_to_erl0
set, which iscsit_release_sessions_for_tpg() and lio_tpg_close_session()
both skip during TPG teardown.  A CLOSESESSION logout also sets
sess->session_logout under sess->conn_lock in iscsit_logout_closesession()
before sess->session_close is set in
iscsit_logout_post_handler_closesession().

Re-checking the full teardown condition (session_close,
session_fall_back_to_erl0, session_logout, and ISCSI_TF_EXPIRED under
se_tpg->session_lock) in iscsi_post_login_handler() would close these
windows as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794732723.git.danjwade95@gmail.com?part=2