qla_target: remove qlt_check_fcport_exist

Comment from original 2012 patch:
  In all our testing this function has never returned true.  However, the
  dropping of hardware_lock necessary to call this function seems to cause
  a use-after-free we manage to hit rather frequently.  Given this
  cost-benefit ratio, I'm willing to remove some 100 lines of code.

And since the same problem exists around shutdown_sess and put_sess,
this patch changes them from taking the hardware_lock to requiring the
hardware_lock to be taken.  In most cases the caller already had the
lock and had to drop it for the called method to reacquire it.  At best
that hurts performance and in rare instances it causes races with fatal
consequences.

We dropped the original 2012 patch when upgrading our kernel and it took
us nearly half a year to discover we still need it.

(nab: Fix qla_tgt_sess reference in tcm_qla2xxx_put_sess)

Signed-off-by: Joern Engel <joern@logfs.org>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

1 files changed, 13 insertions, 138 deletions

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index fcdc22306cab..83a8f7a9ec76 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -544,102 +544,6 @@ out_free_id_list:
         return res;
 }
 
-static bool qlt_check_fcport_exist(struct scsi_qla_host *vha,
-        struct qla_tgt_sess *sess)
-{
-        struct qla_hw_data *ha = vha->hw;
-        struct qla_port_24xx_data *pmap24;
-        bool res, found = false;
-        int rc, i;
-        uint16_t loop_id = 0xFFFF; /* to eliminate compiler's warning */
-        uint16_t entries;
-        void *pmap;
-        int pmap_len;
-        fc_port_t *fcport;
-        int global_resets;
-        unsigned long flags;
-
-retry:
-        global_resets = atomic_read(&ha->tgt.qla_tgt->tgt_global_resets_count);
-
-        rc = qla2x00_get_node_name_list(vha, &pmap, &pmap_len);
-        if (rc != QLA_SUCCESS) {
-                res = false;
-                goto out;
-        }
-
-        pmap24 = pmap;
-        entries = pmap_len/sizeof(*pmap24);
-
-        for (i = 0; i < entries; ++i) {
-                if (!memcmp(sess->port_name, pmap24[i].port_name, WWN_SIZE)) {
-                        loop_id = le16_to_cpu(pmap24[i].loop_id);
-                        found = true;
-                        break;
-                }
-        }
-
-        kfree(pmap);
-
-        if (!found) {
-                res = false;
-                goto out;
-        }
-
-        ql_dbg(ql_dbg_tgt_mgt, vha, 0xf046,
-            "qlt_check_fcport_exist(): loop_id %d", loop_id);
-
-        fcport = kzalloc(sizeof(*fcport), GFP_KERNEL);
-        if (fcport == NULL) {
-                ql_dbg(ql_dbg_tgt_mgt, vha, 0xf047,
-                    "qla_target(%d): Allocation of tmp FC port failed",
-                    vha->vp_idx);
-                res = false;
-                goto out;
-        }
-
-        fcport->loop_id = loop_id;
-
-        rc = qla2x00_get_port_database(vha, fcport, 0);
-        if (rc != QLA_SUCCESS) {
-                ql_dbg(ql_dbg_tgt_mgt, vha, 0xf048,
-                    "qla_target(%d): Failed to retrieve fcport "
-                    "information -- get_port_database() returned %x "
-                    "(loop_id=0x%04x)", vha->vp_idx, rc, loop_id);
-                res = false;
-                goto out_free_fcport;
-        }
-
-        if (global_resets !=
-            atomic_read(&ha->tgt.qla_tgt->tgt_global_resets_count)) {
-                ql_dbg(ql_dbg_tgt_mgt, vha, 0xf002,
-                    "qla_target(%d): global reset during session discovery"
-                    " (counter was %d, new %d), retrying",
-                    vha->vp_idx, global_resets,
-                    atomic_read(&ha->tgt.qla_tgt->tgt_global_resets_count));
-                goto retry;
-        }
-
-        ql_dbg(ql_dbg_tgt_mgt, vha, 0xf003,
-            "Updating sess %p s_id %x:%x:%x, loop_id %d) to d_id %x:%x:%x, "
-            "loop_id %d", sess, sess->s_id.b.domain, sess->s_id.b.al_pa,
-            sess->s_id.b.area, sess->loop_id, fcport->d_id.b.domain,
-            fcport->d_id.b.al_pa, fcport->d_id.b.area, fcport->loop_id);
-
-        spin_lock_irqsave(&ha->hardware_lock, flags);
-        ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
-                                (fcport->flags & FCF_CONF_COMP_SUPPORTED));
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
-
-        res = true;
-
-out_free_fcport:
-        kfree(fcport);
-
-out:
-        return res;
-}
-
 /* ha->hardware_lock supposed to be held on entry */
 static void qlt_undelete_sess(struct qla_tgt_sess *sess)
 {
@@ -663,43 +567,13 @@ static void qlt_del_sess_work_fn(struct delayed_work *work)
                 sess = list_entry(tgt->del_sess_list.next, typeof(*sess),
                     del_list_entry);
                 if (time_after_eq(jiffies, sess->expires)) {
-                        bool cancel;
-
                         qlt_undelete_sess(sess);
 
-                        spin_unlock_irqrestore(&ha->hardware_lock, flags);
-                        cancel = qlt_check_fcport_exist(vha, sess);
-
-                        if (cancel) {
-                                if (sess->deleted) {
-                                        /*
-                                         * sess was again deleted while we were
-                                         * discovering it
-                                         */
-                                        spin_lock_irqsave(&ha->hardware_lock,
-                                            flags);
-                                        continue;
-                                }
-
-                                ql_dbg(ql_dbg_tgt_mgt, vha, 0xf049,
-                                    "qla_target(%d): cancel deletion of "
-                                    "session for port %02x:%02x:%02x:%02x:%02x:"
-                                    "%02x:%02x:%02x (loop ID %d), because "
-                                    " it isn't deleted by firmware",
-                                    vha->vp_idx, sess->port_name[0],
-                                    sess->port_name[1], sess->port_name[2],
-                                    sess->port_name[3], sess->port_name[4],
-                                    sess->port_name[5], sess->port_name[6],
-                                    sess->port_name[7], sess->loop_id);
-                        } else {
-                                ql_dbg(ql_dbg_tgt_mgt, vha, 0xf004,
-                                    "Timeout: sess %p about to be deleted\n",
-                                    sess);
-                                ha->tgt.tgt_ops->shutdown_sess(sess);
-                                ha->tgt.tgt_ops->put_sess(sess);
-                        }
-
-                        spin_lock_irqsave(&ha->hardware_lock, flags);
+                        ql_dbg(ql_dbg_tgt_mgt, vha, 0xf004,
+                            "Timeout: sess %p about to be deleted\n",
+                            sess);
+                        ha->tgt.tgt_ops->shutdown_sess(sess);
+                        ha->tgt.tgt_ops->put_sess(sess);
                 } else {
                         schedule_delayed_work(&tgt->sess_del_work,
                             jiffies - sess->expires);
@@ -884,9 +758,8 @@ void qlt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport)
                     sess->loop_id);
                 sess->local = 0;
         }
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
-
         ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 }
 
 void qlt_fc_port_deleted(struct scsi_qla_host *vha, fc_port_t *fcport)
@@ -2706,7 +2579,9 @@ static void qlt_do_work(struct work_struct *work)
         /*
          * Drop extra session reference from qla_tgt_handle_cmd_for_atio*(
          */
+        spin_lock_irqsave(&ha->hardware_lock, flags);
         ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         return;
 
 out_term:
@@ -2718,9 +2593,9 @@ out_term:
         spin_lock_irqsave(&ha->hardware_lock, flags);
         qlt_send_term_exchange(vha, NULL, &cmd->atio, 1);
         kmem_cache_free(qla_tgt_cmd_cachep, cmd);
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         if (sess)
                 ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 }
 
 /* ha->hardware_lock supposed to be held on entry */
@@ -4169,16 +4044,16 @@ static void qlt_abort_work(struct qla_tgt *tgt,
         rc = __qlt_24xx_handle_abts(vha, &prm->abts, sess);
         if (rc != 0)
                 goto out_term;
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
         ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         return;
 
 out_term:
         qlt_24xx_send_abts_resp(vha, &prm->abts, FCP_TMF_REJECTED, false);
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         if (sess)
                 ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 }
 
 static void qlt_tmr_work(struct qla_tgt *tgt,
@@ -4226,16 +4101,16 @@ static void qlt_tmr_work(struct qla_tgt *tgt,
         rc = qlt_issue_task_mgmt(sess, unpacked_lun, fn, iocb, 0);
         if (rc != 0)
                 goto out_term;
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
         ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         return;
 
 out_term:
         qlt_send_term_exchange(vha, NULL, &prm->tm_iocb2, 1);
-        spin_unlock_irqrestore(&ha->hardware_lock, flags);
         if (sess)
                 ha->tgt.tgt_ops->put_sess(sess);
+        spin_unlock_irqrestore(&ha->hardware_lock, flags);
 }
 
 static void qlt_sess_work_fn(struct work_struct *work)