Re: [PATCH] scsi: target: iscsi: run the Time2Retain handler in process context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] scsi: target: iscsi: UAF race in
  iscsit_stop_time2retain_timer() on restart
- [High] scsi: target: iscsi: session leak in iscsit_close_session()
  when !can_sleep
- [High] scsi: target: iscsi: UAF in iscsit_close_connection() when
  Time2Retain is 0
- [Medium] scsi: target: iscsi: session_lock dropped in
  iscsit_stop_time2retain_timer()
--

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

scsi: target: iscsi: run the Time2Retain handler in process context

This commit converts the session Time2Retain timer into a delayed work item on
system_dfl_wq so that iscsit_handle_time2retain_timeout() closes the session
and acquires se_tpg->session_lock in process context rather than timer softirq
context.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 03d618cd76721..4c6579eb803f3 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -4485,8 +4485,8 @@ int iscsit_close_session(struct iscsit_session *sess, bool can_sleep)
>  
>  	/*
>  	 * If any other processes are accessing this session pointer we must
> -	 * wait until they have completed.  If we are in an interrupt (the
> -	 * time2retain handler) and contain and active session usage count we
> +	 * wait until they have completed.  If we are the Time2Retain work
> +	 * (can_sleep is false) and contain and active session usage count we
>  	 * restart the timer and exit.
>  	 */
>  	if (iscsit_check_session_usage_count(sess, can_sleep)) {

[Severity: High]
Can keeping can_sleep as false when iscsit_handle_time2retain_timeout() calls
iscsit_close_session() leak the session if session_usage_count is non-zero?

In iscsi_target_login_sess_out(), the old_sess_out path starts the Time2Retain
handler and drops se_tpg->session_lock before calling
iscsit_dec_session_usage_count().  When DefaultTime2Retain is 0,
mod_delayed_work() queues time2retain_work with zero delay, so
iscsit_handle_time2retain_timeout() can run on another CPU while
session_usage_count is still elevated.

iscsit_handle_time2retain_timeout() sets ISCSI_TF_EXPIRED (with
ISCSI_TF_RUNNING still set) and calls iscsit_close_session(sess, false).
iscsit_close_session() sets session_reinstatement to 1, fails to stop the
timer because ISCSI_TF_EXPIRED is set, deregisters configfs, and enters the
!can_sleep branch above because session_usage_count is non-zero.  There,
iscsit_start_time2retain_handler() returns immediately without re-arming the
work because ISCSI_TF_RUNNING was never cleared, leaving the half-closed
session leaked with ISCSI_TF_EXPIRED and session_reinstatement set.

Since iscsit_handle_time2retain_timeout() now runs in process context, could
it wait on session_waiting_on_uc_comp instead of taking the non-sleeping
restart path?

[ ... ]

> diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
> index 24db6b07493e6..9db833f3eeaec 100644
> --- a/drivers/target/iscsi/iscsi_target_erl0.c
> +++ b/drivers/target/iscsi/iscsi_target_erl0.c

[ ... ]

> @@ -791,8 +791,8 @@ void iscsit_start_time2retain_handler(struct iscsit_session *sess)
>  
>  	sess->time2retain_timer_flags &= ~ISCSI_TF_STOP;
>  	sess->time2retain_timer_flags |= ISCSI_TF_RUNNING;
> -	mod_timer(&sess->time2retain_timer,
> -		  jiffies + sess->sess_ops->DefaultTime2Retain * HZ);
> +	mod_delayed_work(system_dfl_wq, &sess->time2retain_work,
> +			 sess->sess_ops->DefaultTime2Retain * HZ);
>  }
>  
>  int iscsit_stop_time2retain_timer(struct iscsit_session *sess)

[Severity: High]
Can this cause a use-after-free in iscsit_close_connection() when
DefaultTime2Retain is 0?

drivers/target/iscsi/iscsi_target.c:iscsit_close_connection() {
    ...
		if (!atomic_read(&sess->session_continuation))
			iscsit_start_time2retain_handler(sess);

		if (atomic_read(&sess->session_close)) {
			spin_unlock_bh(&sess->conn_lock);
			complete_all(&sess->session_wait_comp);
			iscsit_close_session(sess, true);
		} else {
			spin_unlock_bh(&sess->conn_lock);
		}
    ...
}

Previously, mod_timer() armed a softirq timer while spin_lock_bh() kept local
softirqs disabled across iscsit_start_time2retain_handler() and the subsequent
sess accesses.  With mod_delayed_work(), a zero delay bypasses the timer wheel
and queues time2retain_work immediately on an unbound worker, and neither
iscsit_handle_time2retain_timeout() nor iscsit_close_session() takes
sess->conn_lock:

CPU0                                  CPU1 (kworker)
iscsit_close_connection()
  spin_lock_bh(&sess->conn_lock)
  iscsit_start_time2retain_handler(sess)
    mod_delayed_work(..., 0)
                                      iscsit_handle_time2retain_timeout()
                                        iscsit_close_session(sess, false)
                                          kfree(sess)
  atomic_read(&sess->session_close)
  spin_unlock_bh(&sess->conn_lock)  <-- use-after-free

> @@ -811,7 +811,7 @@ int iscsit_stop_time2retain_timer(struct iscsit_session *sess)
>  	sess->time2retain_timer_flags |= ISCSI_TF_STOP;
>  	spin_unlock(&se_tpg->session_lock);
>  
> -	timer_delete_sync(&sess->time2retain_timer);
> +	cancel_delayed_work(&sess->time2retain_work);
>  
>  	spin_lock(&se_tpg->session_lock);
>  	sess->time2retain_timer_flags &= ~ISCSI_TF_RUNNING;

[Severity: High]
Can replacing timer_delete_sync() with cancel_delayed_work() here allow an
already-dispatched work item to close and free the session after the timer is
stopped and restarted?

Because cancel_delayed_work() does not wait for a running work item,
iscsit_stop_time2retain_timer() clears ISCSI_TF_RUNNING while a worker thread
can still be preempted before taking se_tpg->session_lock.  If the timer is
then restarted (for example via the old_sess_out path in
iscsi_target_login_sess_out() when a continuation login fails),
iscsit_start_time2retain_handler() clears ISCSI_TF_STOP and re-queues
sess->time2retain_work:

CPU0 (kworker)                        CPU1
iscsit_handle_time2retain_timeout()
  /* preempted before session_lock */
                                      iscsi_login_non_zero_tsih_s2()
                                        iscsit_stop_time2retain_timer()
                                          flags |= ISCSI_TF_STOP
                                          cancel_delayed_work()
                                          flags &= ~ISCSI_TF_RUNNING
                                      ...
                                      iscsi_target_login_sess_out()
                                        iscsit_start_time2retain_handler()
                                          flags &= ~ISCSI_TF_STOP
                                          flags |= ISCSI_TF_RUNNING
                                          mod_delayed_work()
  spin_lock_bh(&se_tpg->session_lock)
  /* ISCSI_TF_STOP is now clear */
  flags |= ISCSI_TF_EXPIRED
  spin_unlock_bh(&se_tpg->session_lock)
  iscsit_close_session(sess, false)
    iscsit_stop_time2retain_timer()   <-- returns -1 (ISCSI_TF_EXPIRED)
    /* cancel_delayed_work_sync() skipped because !can_sleep */
    kfree(sess)                       <-- time2retain_work still queued

When CPU0 acquires se_tpg->session_lock, ISCSI_TF_STOP is no longer set, so
the stale work sets ISCSI_TF_EXPIRED and calls iscsit_close_session(sess,
false).  Because ISCSI_TF_EXPIRED is set and can_sleep is false, neither
iscsit_stop_time2retain_timer() nor cancel_delayed_work_sync() cancels the
newly armed time2retain_work before kfree(sess) frees the session (or if a
new connection is already active on the session, iscsit_close_session() hits
BUG() on sess->nconn).

[Severity: Medium]
Why does iscsit_stop_time2retain_timer() still drop and re-acquire
se_tpg->session_lock around cancel_delayed_work()?

The commit message notes that cancel_delayed_work() is called with
se_tpg->session_lock held because it does not sleep, but the unlock and lock
around it remain from timer_delete_sync().  Dropping se_tpg->session_lock
splits the update of time2retain_timer_flags across two critical sections, so
an unlocked caller of iscsit_start_time2retain_handler() such as
iscsit_close_connection() can observe ISCSI_TF_RUNNING still set in that
window and return without arming the work just before ISCSI_TF_RUNNING is
cleared.

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