Re: [PATCH v2 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 1 potential
issue to consider:
- [Medium] scsi: target: iscsi: session leak when login fails on
  ISCSI_TF_EXPIRED
--

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

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

This commit prevents a non-leading iSCSI login from attaching a connection
to a session that began closing while the login was negotiating.

> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index f4df551817116..556ae5d2d501b 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c

[ ... ]

> @@ -689,7 +713,28 @@ void iscsi_post_login_handler(
>  		iscsi_release_param_list(conn->param_list);
>  		conn->param_list = NULL;
>  
> +		spin_lock_bh(&se_tpg->session_lock);
>  		spin_lock_bh(&sess->conn_lock);
> +		/*
> +		 * iscsi_login_non_zero_tsih_s2() sampled these before the
> +		 * negotiation and never looked again.  Re-test all of them under
> +		 * the lock that serialises against the ->nconn test in
> +		 * iscsit_close_connection(), and before this connection reaches
> +		 * sess_conn_list or TARG_CONN_STATE_LOGGED_IN.
> +		 */
> +		if (atomic_read(&sess->session_close) ||
> +		    atomic_read(&sess->session_fall_back_to_erl0) ||
> +		    atomic_read(&sess->session_logout) ||
> +		    (sess->time2retain_timer_flags & ISCSI_TF_EXPIRED)) {
> +			spin_unlock_bh(&sess->conn_lock);
> +			spin_unlock_bh(&se_tpg->session_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;
> +		}

[Severity: Medium]
Does this leak sess when the login is refused because ISCSI_TF_EXPIRED is
set?

When the Time2Retain timer expires while the negotiating login holds
sess->session_usage_count, iscsit_handle_time2retain_timeout() sets
ISCSI_TF_EXPIRED without clearing ISCSI_TF_RUNNING and calls
iscsit_close_session(sess, false) in timer context.  Because can_sleep is
false, iscsit_check_session_usage_count() does not wait for the login to
drop its reference:

drivers/target/iscsi/iscsi_target.c:iscsit_close_session() {
    ...
	if (iscsit_check_session_usage_count(sess, can_sleep)) {
		atomic_set(&sess->session_logout, 0);
		iscsit_start_time2retain_handler(sess);
		return 0;
	}
    ...
}

iscsit_start_time2retain_handler() then returns early without re-arming
the timer because ISCSI_TF_RUNNING is still set
(iscsit_stop_time2retain_timer() bails out when ISCSI_TF_EXPIRED is set),
and sess->session_reinstatement also remains set.  When
iscsi_post_login_handler() later sees ISCSI_TF_EXPIRED and drops
sess->session_usage_count, iscsit_close_session() has already returned and
nothing ever closes or frees sess.

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