From ef507dba796691eb3a65a99f88e27e2ab727bc7e Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sat, 5 Oct 2013 18:18:56 +0000 Subject: [PATCH] qla2x00t: Fix a recently discovered race condition related to session management (merge r5017, r5018 and r5020 from trunk) git-svn-id: http://svn.code.sf.net/p/scst/svn/branches/2.2.x@5044 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- qla2x00t/qla2x00-target/qla2x00t.c | 231 ++++++++++++++++++++--------- qla2x00t/qla2x00-target/qla2x00t.h | 6 +- scst/include/scst.h | 9 ++ 3 files changed, 174 insertions(+), 72 deletions(-) diff --git a/qla2x00t/qla2x00-target/qla2x00t.c b/qla2x00t/qla2x00-target/qla2x00t.c index f5cce3e38..9c6e3a855 100644 --- a/qla2x00t/qla2x00-target/qla2x00t.c +++ b/qla2x00t/qla2x00-target/qla2x00t.c @@ -99,9 +99,9 @@ static void q24_send_term_exchange(scsi_qla_host_t *ha, struct q2t_cmd *cmd, static void q2t_reject_free_srr_imm(scsi_qla_host_t *ha, struct srr_imm *imm, int ha_lock); static int q2t_cut_cmd_data_head(struct q2t_cmd *cmd, unsigned int offset); -static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool local_only); +static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool immediately); static void q2t_on_hw_pending_cmd_timeout(struct scst_cmd *scst_cmd); -static int q2t_unreg_sess(struct q2t_sess *sess); +static void q2t_unreg_sess(struct q2t_sess *sess); static uint16_t q2t_get_scsi_transport_version(struct scst_tgt *scst_tgt); static uint16_t q2t_get_phys_transport_version(struct scst_tgt *scst_tgt); @@ -253,7 +253,12 @@ static inline int scst_cmd_get_ppl_offset(struct scst_cmd *scst_cmd) return 0; } -/* pha->hardware_lock supposed to be held on entry */ +/* + * pha->hardware_lock supposed to be held on entry. + * + * !! If you are calling it after finding sess in tgt->sess_list, make sure + * !! that the lock is not dropped between find and this function call! + */ static inline void q2t_sess_get(struct q2t_sess *sess) { sess->sess_ref++; @@ -277,8 +282,10 @@ static inline struct q2t_sess *q2t_find_sess_by_loop_id(struct q2t_tgt *tgt, { struct q2t_sess *sess; list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { - if ((loop_id == sess->loop_id) && !sess->deleted) + if (loop_id == sess->loop_id) { + EXTRACHECKS_BUG_ON(sess->deleted); return sess; + } } return NULL; } @@ -291,8 +298,18 @@ static inline struct q2t_sess *q2t_find_sess_by_s_id_include_deleted( list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { if ((sess->s_id.b.al_pa == s_id[2]) && (sess->s_id.b.area == s_id[1]) && - (sess->s_id.b.domain == s_id[0])) + (sess->s_id.b.domain == s_id[0])) { + EXTRACHECKS_BUG_ON(sess->deleted); return sess; + } + } + list_for_each_entry(sess, &tgt->del_sess_list, sess_list_entry) { + if ((sess->s_id.b.al_pa == s_id[2]) && + (sess->s_id.b.area == s_id[1]) && + (sess->s_id.b.domain == s_id[0])) { + EXTRACHECKS_BUG_ON(!sess->deleted); + return sess; + } } return NULL; } @@ -305,9 +322,10 @@ static inline struct q2t_sess *q2t_find_sess_by_s_id(struct q2t_tgt *tgt, list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { if ((sess->s_id.b.al_pa == s_id[2]) && (sess->s_id.b.area == s_id[1]) && - (sess->s_id.b.domain == s_id[0]) && - !sess->deleted) + (sess->s_id.b.domain == s_id[0])) { + EXTRACHECKS_BUG_ON(sess->deleted); return sess; + } } return NULL; } @@ -320,9 +338,10 @@ static inline struct q2t_sess *q2t_find_sess_by_s_id_le(struct q2t_tgt *tgt, list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { if ((sess->s_id.b.al_pa == s_id[0]) && (sess->s_id.b.area == s_id[1]) && - (sess->s_id.b.domain == s_id[2]) && - !sess->deleted) + (sess->s_id.b.domain == s_id[2])) { + EXTRACHECKS_BUG_ON(sess->deleted); return sess; + } } return NULL; } @@ -340,8 +359,44 @@ static inline struct q2t_sess *q2t_find_sess_by_port_name(struct q2t_tgt *tgt, (sess->port_name[4] == port_name[4]) && (sess->port_name[5] == port_name[5]) && (sess->port_name[6] == port_name[6]) && - (sess->port_name[7] == port_name[7])) + (sess->port_name[7] == port_name[7])) { + EXTRACHECKS_BUG_ON(sess->deleted); return sess; + } + } + return NULL; +} + +/* pha->hardware_lock supposed to be held on entry (to protect tgt->sess_list) */ +static inline struct q2t_sess *q2t_find_sess_by_port_name_include_deleted( + struct q2t_tgt *tgt, const uint8_t *port_name) +{ + struct q2t_sess *sess; + list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { + if ((sess->port_name[0] == port_name[0]) && + (sess->port_name[1] == port_name[1]) && + (sess->port_name[2] == port_name[2]) && + (sess->port_name[3] == port_name[3]) && + (sess->port_name[4] == port_name[4]) && + (sess->port_name[5] == port_name[5]) && + (sess->port_name[6] == port_name[6]) && + (sess->port_name[7] == port_name[7])) { + EXTRACHECKS_BUG_ON(sess->deleted); + return sess; + } + } + list_for_each_entry(sess, &tgt->del_sess_list, sess_list_entry) { + if ((sess->port_name[0] == port_name[0]) && + (sess->port_name[1] == port_name[1]) && + (sess->port_name[2] == port_name[2]) && + (sess->port_name[3] == port_name[3]) && + (sess->port_name[4] == port_name[4]) && + (sess->port_name[5] == port_name[5]) && + (sess->port_name[6] == port_name[6]) && + (sess->port_name[7] == port_name[7])) { + EXTRACHECKS_BUG_ON(!sess->deleted); + return sess; + } } return NULL; } @@ -656,20 +711,16 @@ out: } /* pha->hardware_lock supposed to be held on entry */ -static int q2t_unreg_sess(struct q2t_sess *sess) +static void q2t_unreg_sess(struct q2t_sess *sess) { - int res = 1; - TRACE_ENTRY(); sBUG_ON(sess == NULL); sBUG_ON(sess->sess_ref != 0); - TRACE_MGMT_DBG("Deleting sess %p from tgt %p", sess, sess->tgt); - list_del(&sess->sess_list_entry); + TRACE_MGMT_DBG("Unregistering sess %p (tgt %p)", sess, sess->tgt); - if (sess->deleted) - list_del(&sess->del_list_entry); + sBUG_ON(list_entry_in_list(&sess->sess_list_entry)); PRINT_INFO("qla2x00t(%ld): %ssession for loop_id %d deleted", sess->tgt->ha->instance, sess->local ? "local " : "", @@ -677,8 +728,8 @@ static int q2t_unreg_sess(struct q2t_sess *sess) scst_unregister_session(sess->scst_sess, 0, q2t_free_session_done); - TRACE_EXIT_RES(res); - return res; + TRACE_EXIT(); + return; } /* pha->hardware_lock supposed to be held on entry */ @@ -700,7 +751,7 @@ static int q2t_reset(scsi_qla_host_t *ha, void *iocb, int mcmd) if (loop_id == 0xFFFF) { /* Global event */ atomic_inc(&ha->tgt->tgt_global_resets_count); - q2t_clear_tgt_db(ha->tgt, 1); + q2t_clear_tgt_db(ha->tgt, false); if (!list_empty(&ha->tgt->sess_list)) { sess = list_first_entry(&ha->tgt->sess_list, typeof(*sess), sess_list_entry); @@ -752,6 +803,21 @@ out: return res; } +/* pha->hardware_lock supposed to be held on entry */ +static void q2t_sess_del(struct q2t_sess *sess) +{ + TRACE_ENTRY(); + + TRACE_MGMT_DBG("Deleting sess %p", sess); + + sBUG_ON(!list_entry_in_list(&sess->sess_list_entry)); + list_del_init(&sess->sess_list_entry); + q2t_sess_put(sess); + + TRACE_EXIT(); + return; +} + /* pha->hardware_lock supposed to be held on entry */ static void q2t_schedule_sess_for_deletion(struct q2t_sess *sess) { @@ -764,6 +830,13 @@ static void q2t_schedule_sess_for_deletion(struct q2t_sess *sess) if (sess->deleted) goto out; + if (tgt->tgt_stop) { + TRACE_DBG("tgt %p stopped, deleting sess %p immediately", + tgt, sess); + q2t_sess_del(sess); + goto out; + } + /* * If the list is empty, then, most likely, the work isn't * scheduled. @@ -772,7 +845,7 @@ static void q2t_schedule_sess_for_deletion(struct q2t_sess *sess) TRACE_MGMT_DBG("Scheduling sess %p for deletion (schedule %d)", sess, schedule); - list_add_tail(&sess->del_list_entry, &tgt->del_sess_list); + list_move_tail(&sess->sess_list_entry, &tgt->del_sess_list); sess->deleted = 1; sess->expires = jiffies + dev_loss_tmo * HZ; @@ -795,7 +868,7 @@ out: } /* pha->hardware_lock supposed to be held on entry */ -static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool local_only) +static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool immediately) { struct q2t_sess *sess, *sess_tmp; @@ -805,12 +878,21 @@ static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool local_only) list_for_each_entry_safe(sess, sess_tmp, &tgt->sess_list, sess_list_entry) { - if (local_only) { - if (!sess->local) - continue; + sBUG_ON(sess->deleted); + + if (immediately) + q2t_sess_del(sess); + else q2t_schedule_sess_for_deletion(sess); - } else - q2t_sess_put(sess); + } + + if (immediately) { + /* Delete also scheduled to del sessions */ + list_for_each_entry_safe(sess, sess_tmp, &tgt->del_sess_list, + sess_list_entry) { + sBUG_ON(!sess->deleted); + q2t_sess_del(sess); + } } /* At this point tgt could be already dead */ @@ -825,23 +907,27 @@ static void q2t_clear_tgt_db(struct q2t_tgt *tgt, bool local_only) static void q2t_alloc_session_done(struct scst_session *scst_sess, void *data, int result) { + struct q2t_sess *sess = (struct q2t_sess *)data; + struct q2t_tgt *tgt = sess->tgt; + scsi_qla_host_t *ha = tgt->ha; + scsi_qla_host_t *pha = to_qla_parent(ha); + unsigned long flags; + TRACE_ENTRY(); - if (result != 0) { - struct q2t_sess *sess = (struct q2t_sess *)data; - struct q2t_tgt *tgt = sess->tgt; - scsi_qla_host_t *ha = tgt->ha; - scsi_qla_host_t *pha = to_qla_parent(ha); - unsigned long flags; + spin_lock_irqsave(&pha->hardware_lock, flags); + if (result != 0) { PRINT_INFO("qla2x00t(%ld): Session initialization failed", ha->instance); - - spin_lock_irqsave(&pha->hardware_lock, flags); - q2t_sess_put(sess); - spin_unlock_irqrestore(&pha->hardware_lock, flags); + if (list_entry_in_list(&sess->sess_list_entry)) + q2t_sess_del(sess); } + q2t_sess_put(sess); + + spin_unlock_irqrestore(&pha->hardware_lock, flags); + TRACE_EXIT(); return; } @@ -1026,10 +1112,17 @@ out: /* pha->hardware_lock supposed to be held on entry */ static void q2t_undelete_sess(struct q2t_sess *sess) { - sBUG_ON(!sess->deleted); + TRACE_ENTRY(); - list_del(&sess->del_list_entry); + sBUG_ON(!sess->deleted); + sBUG_ON(!list_entry_in_list(&sess->sess_list_entry)); + + TRACE_MGMT_DBG("Undeleting sess %p", sess); + list_move(&sess->sess_list_entry, &sess->tgt->sess_list); sess->deleted = 0; + + TRACE_EXIT(); + return; } static void q2t_del_sess_work_fn(struct delayed_work *work) @@ -1046,17 +1139,15 @@ static void q2t_del_sess_work_fn(struct delayed_work *work) spin_lock_irqsave(&pha->hardware_lock, flags); while (!list_empty(&tgt->del_sess_list)) { sess = list_first_entry(&tgt->del_sess_list, typeof(*sess), - del_list_entry); + sess_list_entry); if (time_after_eq(jiffies, sess->expires)) { bool cancel; - q2t_undelete_sess(sess); - /* * We need to take extra reference, because we are * going to drop hardware_lock. Otherwise, we are racing * with other possible callers of q2t_sess_put() for - * the same sess, e.g. by q2t_clear_tgt_db(). + * the same sess. */ q2t_sess_get(sess); @@ -1064,14 +1155,17 @@ static void q2t_del_sess_work_fn(struct delayed_work *work) cancel = q2t_check_fcport_exist(ha, sess); spin_lock_irqsave(&pha->hardware_lock, flags); + if (!sess->deleted || + !list_entry_in_list(&sess->sess_list_entry)) { + /* + * session has been undeleted or got ready to + * be destroyed while we were entering here + */ + goto put_continue; + } + if (cancel) { - if (sess->deleted) { - /* - * sess was again deleted while we were - * discovering it - */ - goto put_continue; - } + q2t_undelete_sess(sess); PRINT_INFO("qla2x00t(%ld): cancel deletion of " "session for port %02x:%02x:%02x:" @@ -1084,9 +1178,8 @@ static void q2t_del_sess_work_fn(struct delayed_work *work) sess->port_name[6], sess->port_name[7], sess->loop_id); } else { - TRACE_MGMT_DBG("Timeout: sess %p about to be " - "deleted", sess); - q2t_sess_put(sess); + TRACE_MGMT_DBG("Timeout for sess %p", sess); + q2t_sess_del(sess); } put_continue: q2t_sess_put(sess); @@ -1120,15 +1213,8 @@ static struct q2t_sess *q2t_create_sess(scsi_qla_host_t *ha, fc_port_t *fcport, /* Check to avoid double sessions */ spin_lock_irq(&pha->hardware_lock); - list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) { - if ((sess->port_name[0] == fcport->port_name[0]) && - (sess->port_name[1] == fcport->port_name[1]) && - (sess->port_name[2] == fcport->port_name[2]) && - (sess->port_name[3] == fcport->port_name[3]) && - (sess->port_name[4] == fcport->port_name[4]) && - (sess->port_name[5] == fcport->port_name[5]) && - (sess->port_name[6] == fcport->port_name[6]) && - (sess->port_name[7] == fcport->port_name[7])) { + sess = q2t_find_sess_by_port_name_include_deleted(tgt, fcport->port_name); + if (sess != NULL) { TRACE_MGMT_DBG("Double sess %p found (s_id %x:%x:%x, " "loop_id %d), updating to d_id %x:%x:%x, " "loop_id %d", sess, sess->s_id.b.domain, @@ -1148,7 +1234,6 @@ static struct q2t_sess *q2t_create_sess(scsi_qla_host_t *ha, fc_port_t *fcport, sess->local = 0; spin_unlock_irq(&pha->hardware_lock); goto out; - } } spin_unlock_irq(&pha->hardware_lock); @@ -1166,7 +1251,8 @@ static struct q2t_sess *q2t_create_sess(scsi_qla_host_t *ha, fc_port_t *fcport, goto out; } - sess->sess_ref = 2; /* plus 1 extra ref, see above */ + /* +1 for q2t_alloc_session_done(), +1 extra ref, see above */ + sess->sess_ref = 3; sess->tgt = ha->tgt; sess->s_id = fcport->d_id; sess->loop_id = fcport->loop_id; @@ -1192,7 +1278,12 @@ static struct q2t_sess *q2t_create_sess(scsi_qla_host_t *ha, fc_port_t *fcport, goto out_free_sess; } - /* Let's do the session creation async'ly */ + /* Lock here to eliminate creating sessions for being stopped target */ + spin_lock_irq(&pha->hardware_lock); + + if (tgt->tgt_stop) + goto out_free_sess_wwn; + sess->scst_sess = scst_register_session(tgt->scst_tgt, 1, wwn_str, sess, sess, q2t_alloc_session_done); if (sess->scst_sess == NULL) { @@ -1204,10 +1295,9 @@ static struct q2t_sess *q2t_create_sess(scsi_qla_host_t *ha, fc_port_t *fcport, } TRACE_MGMT_DBG("Adding sess %p to tgt %p", sess, tgt); - - spin_lock_irq(&pha->hardware_lock); list_add_tail(&sess->sess_list_entry, &tgt->sess_list); tgt->sess_count++; + spin_unlock_irq(&pha->hardware_lock); PRINT_INFO("qla2x00t(%ld): %ssession for wwn %s (loop_id %d, " @@ -1223,6 +1313,8 @@ out: return sess; out_free_sess_wwn: + spin_unlock_irq(&pha->hardware_lock); + kfree(wwn_str); /* go through */ @@ -1252,7 +1344,7 @@ static void q2t_fc_port_added(scsi_qla_host_t *ha, fc_port_t *fcport) spin_lock_irq(&pha->hardware_lock); - sess = q2t_find_sess_by_port_name(tgt, fcport->port_name); + sess = q2t_find_sess_by_port_name_include_deleted(tgt, fcport->port_name); if (sess == NULL) { spin_unlock_irq(&pha->hardware_lock); sess = q2t_create_sess(ha, fcport, false); @@ -1377,7 +1469,7 @@ static void q2t_target_stop(struct scst_tgt *scst_tgt) mutex_lock(&ha->tgt_mutex); spin_lock_irq(&pha->hardware_lock); tgt->tgt_stop = 1; - q2t_clear_tgt_db(tgt, false); + q2t_clear_tgt_db(tgt, true); spin_unlock_irq(&pha->hardware_lock); mutex_unlock(&ha->tgt_mutex); @@ -4092,7 +4184,6 @@ static int q24_handle_els(scsi_qla_host_t *ha, notify24xx_entry_t *iocb) if (loop_id != 0xFFFF) { sess = q2t_find_sess_by_loop_id(ha->tgt, loop_id); - if (sess) { TRACE_MGMT_DBG("qla2x00t(%ld): port logged out - scheduling session %p for deletion " "(port %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x, loop_id %d)", diff --git a/qla2x00t/qla2x00-target/qla2x00t.h b/qla2x00t/qla2x00-target/qla2x00t.h index 4cb59c004..f7b308ba8 100644 --- a/qla2x00t/qla2x00-target/qla2x00t.h +++ b/qla2x00t/qla2x00-target/qla2x00t.h @@ -141,7 +141,10 @@ struct q2t_tgt { /* Count of sessions refering q2t_tgt. Protected by hardware_lock. */ int sess_count; - /* Protected by hardware_lock. Addition also protected by tgt_mutex. */ + /* + * Protected by hardware_lock. Adding new sessions (not undelete) + * also protected by tgt_mutex. + */ struct list_head sess_list; /* Protected by hardware_lock */ @@ -188,7 +191,6 @@ struct q2t_sess { struct list_head sess_list_entry; unsigned long expires; - struct list_head del_list_entry; uint8_t port_name[WWN_SIZE]; }; diff --git a/scst/include/scst.h b/scst/include/scst.h index c842d2699..8a570a68f 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -150,6 +150,15 @@ static inline unsigned int queue_max_hw_sectors(struct request_queue *q) #define __list_for_each list_for_each #endif +/* + * Returns true if entry is in its list. Entry must be deleted from the + * list by using list_del_init()! + */ +static inline bool list_entry_in_list(const struct list_head *entry) +{ + return !list_empty(entry); +} + #define SCST_INTERFACE_VERSION \ SCST_VERSION_STRING "$Revision$" SCST_CONST_VERSION