On 4/15/21 7:04 PM, Mike Christie wrote:
> If we haven't done a unbind target call we can race where
> iscsi_conn_teardown wakes up the eh/abort thread and then frees the conn
> while those threads are still accessing the conn ehwait.
>
> We can only do one TMF per session so this just moves the TMF fields from
> the conn to the session. We can then rely on the
> iscsi_session_teardown->iscsi_remove_session->__iscsi_unbind_session call
> to remove the target and it's devices, and know after that point there is
> no device or scsi-ml callout trying to access the session.
>
> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
> ---
> drivers/scsi/libiscsi.c | 123 +++++++++++++++++++---------------------
> include/scsi/libiscsi.h | 11 ++--
> 2 files changed, 64 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ce6d04035c64..56b41d8fff02 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -230,11 +230,11 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_task *task)
> */
> static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
> {
> - struct iscsi_conn *conn = task->conn;
> - struct iscsi_tm *tmf = &conn->tmhdr;
> + struct iscsi_session *session = task->conn->session;
> + struct iscsi_tm *tmf = &session->tmhdr;
> u64 hdr_lun;
>
> - if (conn->tmf_state == TMF_INITIAL)
> + if (session->tmf_state == TMF_INITIAL)
> return 0;
>
> if ((tmf->opcode & ISCSI_OPCODE_MASK) != ISCSI_OP_SCSI_TMFUNC)
> @@ -254,24 +254,19 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
> * Fail all SCSI cmd PDUs
> */
> if (opcode != ISCSI_OP_SCSI_DATA_OUT) {
> - iscsi_conn_printk(KERN_INFO, conn,
> - "task [op %x itt "
> - "0x%x/0x%x] "
> - "rejected.\n",
> - opcode, task->itt,
> - task->hdr_itt);
> + iscsi_session_printk(KERN_INFO, session,
> + "task [op %x itt 0x%x/0x%x] rejected.\n",
> + opcode, task->itt, task->hdr_itt);
> return -EACCES;
> }
> /*
> * And also all data-out PDUs in response to R2T
> * if fast_abort is set.
> */
> - if (conn->session->fast_abort) {
> - iscsi_conn_printk(KERN_INFO, conn,
> - "task [op %x itt "
> - "0x%x/0x%x] fast abort.\n",
> - opcode, task->itt,
> - task->hdr_itt);
> + if (session->fast_abort) {
> + iscsi_session_printk(KERN_INFO, session,
> + "task [op %x itt 0x%x/0x%x] fast abort.\n",
> + opcode, task->itt, task->hdr_itt);
> return -EACCES;
> }
> break;
> @@ -284,7 +279,7 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
> */
> if (opcode == ISCSI_OP_SCSI_DATA_OUT &&
> task->hdr_itt == tmf->rtt) {
> - ISCSI_DBG_SESSION(conn->session,
> + ISCSI_DBG_SESSION(session,
> "Preventing task %x/%x from sending "
> "data-out due to abort task in "
> "progress\n", task->itt,
> @@ -936,20 +931,21 @@ iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
> static void iscsi_tmf_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
> {
> struct iscsi_tm_rsp *tmf = (struct iscsi_tm_rsp *)hdr;
> + struct iscsi_session *session = conn->session;
>
> conn->exp_statsn = be32_to_cpu(hdr->statsn) + 1;
> conn->tmfrsp_pdus_cnt++;
>
> - if (conn->tmf_state != TMF_QUEUED)
> + if (session->tmf_state != TMF_QUEUED)
> return;
>
> if (tmf->response == ISCSI_TMF_RSP_COMPLETE)
> - conn->tmf_state = TMF_SUCCESS;
> + session->tmf_state = TMF_SUCCESS;
> else if (tmf->response == ISCSI_TMF_RSP_NO_TASK)
> - conn->tmf_state = TMF_NOT_FOUND;
> + session->tmf_state = TMF_NOT_FOUND;
> else
> - conn->tmf_state = TMF_FAILED;
> - wake_up(&conn->ehwait);
> + session->tmf_state = TMF_FAILED;
> + wake_up(&session->ehwait);
> }
>
> static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
> @@ -1734,20 +1730,20 @@ static bool iscsi_eh_running(struct iscsi_conn *conn, struct scsi_cmnd *sc,
> * same cmds. Once we get a TMF that can affect multiple cmds stop
> * queueing.
> */
> - if (conn->tmf_state != TMF_INITIAL) {
> - tmf = &conn->tmhdr;
> + if (session->tmf_state != TMF_INITIAL) {
> + tmf = &session->tmhdr;
>
> switch (ISCSI_TM_FUNC_VALUE(tmf)) {
> case ISCSI_TM_FUNC_LOGICAL_UNIT_RESET:
> if (sc->device->lun != scsilun_to_int(&tmf->lun))
> break;
>
> - ISCSI_DBG_EH(conn->session,
> + ISCSI_DBG_EH(session,
> "Requeue cmd sent during LU RESET processing.\n");
> sc->result = DID_REQUEUE << 16;
> goto eh_running;
> case ISCSI_TM_FUNC_TARGET_WARM_RESET:
> - ISCSI_DBG_EH(conn->session,
> + ISCSI_DBG_EH(session,
> "Requeue cmd sent during TARGET RESET processing.\n");
> sc->result = DID_REQUEUE << 16;
> goto eh_running;
> @@ -1866,15 +1862,14 @@ EXPORT_SYMBOL_GPL(iscsi_target_alloc);
>
> static void iscsi_tmf_timedout(struct timer_list *t)
> {
> - struct iscsi_conn *conn = from_timer(conn, t, tmf_timer);
> - struct iscsi_session *session = conn->session;
> + struct iscsi_session *session = from_timer(session, t, tmf_timer);
>
> spin_lock(&session->frwd_lock);
> - if (conn->tmf_state == TMF_QUEUED) {
> - conn->tmf_state = TMF_TIMEDOUT;
> + if (session->tmf_state == TMF_QUEUED) {
> + session->tmf_state = TMF_TIMEDOUT;
> ISCSI_DBG_EH(session, "tmf timedout\n");
> /* unblock eh_abort() */
> - wake_up(&conn->ehwait);
> + wake_up(&session->ehwait);
> }
> spin_unlock(&session->frwd_lock);
> }
> @@ -1897,8 +1892,8 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
> return -EPERM;
> }
> conn->tmfcmd_pdus_cnt++;
> - conn->tmf_timer.expires = timeout * HZ + jiffies;
> - add_timer(&conn->tmf_timer);
> + session->tmf_timer.expires = timeout * HZ + jiffies;
> + add_timer(&session->tmf_timer);
> ISCSI_DBG_EH(session, "tmf set timeout\n");
>
> spin_unlock_bh(&session->frwd_lock);
> @@ -1912,12 +1907,12 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
> * 3) session is terminated or restarted or userspace has
> * given up on recovery
> */
> - wait_event_interruptible(conn->ehwait, age != session->age ||
> + wait_event_interruptible(session->ehwait, age != session->age ||
> session->state != ISCSI_STATE_LOGGED_IN ||
> - conn->tmf_state != TMF_QUEUED);
> + session->tmf_state != TMF_QUEUED);
> if (signal_pending(current))
> flush_signals(current);
> - del_timer_sync(&conn->tmf_timer);
> + del_timer_sync(&session->tmf_timer);
>
> mutex_lock(&session->eh_mutex);
> spin_lock_bh(&session->frwd_lock);
> @@ -2347,17 +2342,17 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> }
>
> /* only have one tmf outstanding at a time */
> - if (conn->tmf_state != TMF_INITIAL)
> + if (session->tmf_state != TMF_INITIAL)
> goto failed;
> - conn->tmf_state = TMF_QUEUED;
> + session->tmf_state = TMF_QUEUED;
>
> - hdr = &conn->tmhdr;
> + hdr = &session->tmhdr;
> iscsi_prep_abort_task_pdu(task, hdr);
>
> if (iscsi_exec_task_mgmt_fn(conn, hdr, age, session->abort_timeout))
> goto failed;
>
> - switch (conn->tmf_state) {
> + switch (session->tmf_state) {
> case TMF_SUCCESS:
> spin_unlock_bh(&session->frwd_lock);
> /*
> @@ -2372,7 +2367,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> */
> spin_lock_bh(&session->frwd_lock);
> fail_scsi_task(task, DID_ABORT);
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> memset(hdr, 0, sizeof(*hdr));
> spin_unlock_bh(&session->frwd_lock);
> iscsi_start_tx(conn);
> @@ -2383,7 +2378,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> goto failed_unlocked;
> case TMF_NOT_FOUND:
> if (!sc->SCp.ptr) {
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> memset(hdr, 0, sizeof(*hdr));
> /* task completed before tmf abort response */
> ISCSI_DBG_EH(session, "sc completed while abort in "
> @@ -2392,7 +2387,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> }
> fallthrough;
> default:
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> goto failed;
> }
>
> @@ -2451,11 +2446,11 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
> conn = session->leadconn;
>
> /* only have one tmf outstanding at a time */
> - if (conn->tmf_state != TMF_INITIAL)
> + if (session->tmf_state != TMF_INITIAL)
> goto unlock;
> - conn->tmf_state = TMF_QUEUED;
> + session->tmf_state = TMF_QUEUED;
>
> - hdr = &conn->tmhdr;
> + hdr = &session->tmhdr;
> iscsi_prep_lun_reset_pdu(sc, hdr);
>
> if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age,
> @@ -2464,7 +2459,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
> goto unlock;
> }
>
> - switch (conn->tmf_state) {
> + switch (session->tmf_state) {
> case TMF_SUCCESS:
> break;
> case TMF_TIMEDOUT:
> @@ -2472,7 +2467,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
> goto done;
> default:
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> goto unlock;
> }
>
> @@ -2484,7 +2479,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
> spin_lock_bh(&session->frwd_lock);
> memset(hdr, 0, sizeof(*hdr));
> fail_scsi_tasks(conn, sc->device->lun, DID_ERROR);
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> spin_unlock_bh(&session->frwd_lock);
>
> iscsi_start_tx(conn);
> @@ -2507,8 +2502,7 @@ void iscsi_session_recovery_timedout(struct iscsi_cls_session *cls_session)
> spin_lock_bh(&session->frwd_lock);
> if (session->state != ISCSI_STATE_LOGGED_IN) {
> session->state = ISCSI_STATE_RECOVERY_FAILED;
> - if (session->leadconn)
> - wake_up(&session->leadconn->ehwait);
> + wake_up(&session->ehwait);
> }
> spin_unlock_bh(&session->frwd_lock);
> }
> @@ -2553,7 +2547,7 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
>
> ISCSI_DBG_EH(session, "wait for relogin\n");
> - wait_event_interruptible(conn->ehwait,
> + wait_event_interruptible(session->ehwait,
> session->state == ISCSI_STATE_TERMINATE ||
> session->state == ISCSI_STATE_LOGGED_IN ||
> session->state == ISCSI_STATE_RECOVERY_FAILED);
> @@ -2614,11 +2608,11 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
> conn = session->leadconn;
>
> /* only have one tmf outstanding at a time */
> - if (conn->tmf_state != TMF_INITIAL)
> + if (session->tmf_state != TMF_INITIAL)
> goto unlock;
> - conn->tmf_state = TMF_QUEUED;
> + session->tmf_state = TMF_QUEUED;
>
> - hdr = &conn->tmhdr;
> + hdr = &session->tmhdr;
> iscsi_prep_tgt_reset_pdu(sc, hdr);
>
> if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age,
> @@ -2627,7 +2621,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
> goto unlock;
> }
>
> - switch (conn->tmf_state) {
> + switch (session->tmf_state) {
> case TMF_SUCCESS:
> break;
> case TMF_TIMEDOUT:
> @@ -2635,7 +2629,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
> goto done;
> default:
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> goto unlock;
> }
>
> @@ -2647,7 +2641,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
> spin_lock_bh(&session->frwd_lock);
> memset(hdr, 0, sizeof(*hdr));
> fail_scsi_tasks(conn, -1, DID_ERROR);
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> spin_unlock_bh(&session->frwd_lock);
>
> iscsi_start_tx(conn);
> @@ -2977,7 +2971,10 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
> session->tt = iscsit;
> session->dd_data = cls_session->dd_data + sizeof(*session);
>
> + session->tmf_state = TMF_INITIAL;
> + timer_setup(&session->tmf_timer, iscsi_tmf_timedout, 0);
> mutex_init(&session->eh_mutex);
> +
> spin_lock_init(&session->frwd_lock);
> spin_lock_init(&session->back_lock);
>
> @@ -3081,7 +3078,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
> conn->c_stage = ISCSI_CONN_INITIAL_STAGE;
> conn->id = conn_idx;
> conn->exp_statsn = 0;
> - conn->tmf_state = TMF_INITIAL;
>
> timer_setup(&conn->transport_timer, iscsi_check_transport_timeouts, 0);
>
> @@ -3106,8 +3102,7 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
> goto login_task_data_alloc_fail;
> conn->login_task->data = conn->data = data;
>
> - timer_setup(&conn->tmf_timer, iscsi_tmf_timedout, 0);
> - init_waitqueue_head(&conn->ehwait);
> + init_waitqueue_head(&session->ehwait);
>
> return cls_conn;
>
> @@ -3160,7 +3155,7 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
> * leading connection? then give up on recovery.
> */
> session->state = ISCSI_STATE_TERMINATE;
> - wake_up(&conn->ehwait);
> + wake_up(&session->ehwait);
> }
>
> spin_unlock_bh(&session->frwd_lock);
> @@ -3245,7 +3240,7 @@ int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
> * commands after successful recovery
> */
> conn->stop_stage = 0;
> - conn->tmf_state = TMF_INITIAL;
> + session->tmf_state = TMF_INITIAL;
> session->age++;
> if (session->age == 16)
> session->age = 0;
> @@ -3259,7 +3254,7 @@ int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
> spin_unlock_bh(&session->frwd_lock);
>
> iscsi_unblock_session(session->cls_session);
> - wake_up(&conn->ehwait);
> + wake_up(&session->ehwait);
> return 0;
> }
> EXPORT_SYMBOL_GPL(iscsi_conn_start);
> @@ -3353,7 +3348,7 @@ void iscsi_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
> spin_lock_bh(&session->frwd_lock);
> fail_scsi_tasks(conn, -1, DID_TRANSPORT_DISRUPTED);
> fail_mgmt_tasks(session, conn);
> - memset(&conn->tmhdr, 0, sizeof(conn->tmhdr));
> + memset(&session->tmhdr, 0, sizeof(session->tmhdr));
> spin_unlock_bh(&session->frwd_lock);
> mutex_unlock(&session->eh_mutex);
> }
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index ec6d508e7a4a..545dfefffe9b 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -202,12 +202,6 @@ struct iscsi_conn {
> unsigned long suspend_tx; /* suspend Tx */
> unsigned long suspend_rx; /* suspend Rx */
>
> - /* abort */
> - wait_queue_head_t ehwait; /* used in eh_abort() */
> - struct iscsi_tm tmhdr;
> - struct timer_list tmf_timer;
> - int tmf_state; /* see TMF_INITIAL, etc.*/
> -
> /* negotiated params */
> unsigned max_recv_dlength; /* initiator_max_recv_dsl*/
> unsigned max_xmit_dlength; /* target_max_recv_dsl */
> @@ -277,6 +271,11 @@ struct iscsi_session {
> * and recv lock.
> */
> struct mutex eh_mutex;
> + /* abort */
> + wait_queue_head_t ehwait; /* used in eh_abort() */
> + struct iscsi_tm tmhdr;
> + struct timer_list tmf_timer;
> + int tmf_state; /* see TMF_INITIAL, etc.*/
>
> /* iSCSI session-wide sequencing */
> uint32_t cmdsn;
>
Reviewed-by: Lee Duncan <lduncan@xxxxxxxx>
