scsi: iscsi: Fix conn use after free during resets

If we haven't done a unbind target call we can race where
iscsi_conn_teardown wakes up the EH 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.

Link: https://lore.kernel.org/r/20210525181821.7617-14-michael.christie@oracle.com
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b7445d9e99d675face8229b303a17693eb6e8133..94abb093098d2320c183fde68a25bc760f3bd9c8 100644 (file)
--- 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)
@@ -1826,15 +1822,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);
 }
@@ -1857,8 +1852,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);
@@ -1872,12 +1867,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);
@@ -2308,17 +2303,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);
                /*
@@ -2333,7 +2328,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);
@@ -2344,7 +2339,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 "
@@ -2353,7 +2348,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
                }
                fallthrough;
        default:
-               conn->tmf_state = TMF_INITIAL;
+               session->tmf_state = TMF_INITIAL;
                goto failed;
        }
 
@@ -2414,11 +2409,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,
@@ -2427,7 +2422,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:
@@ -2435,7 +2430,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;
        }
 
@@ -2447,7 +2442,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);
@@ -2470,8 +2465,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);
 }
@@ -2516,7 +2510,7 @@ failed:
        iscsi_put_conn(conn->cls_conn);
 
        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);
@@ -2577,11 +2571,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,
@@ -2590,7 +2584,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:
@@ -2598,7 +2592,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;
        }
 
@@ -2610,7 +2604,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);
@@ -2940,7 +2934,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);
 
@@ -3044,7 +3041,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);
 
@@ -3069,8 +3065,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;
 
@@ -3105,7 +3100,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);
 
@@ -3180,7 +3175,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;
@@ -3194,7 +3189,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);
@@ -3288,7 +3283,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 13d413a0b8b62ac7a95afd6f11ed055e7a7d4a6d..9d7908265afea1c1399b0dff95ced7d72c2631a9 100644 (file)
--- 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;