Skip to contentSkip to conversation

zfcp fixes for scsi-misc

PATCH New Christof Schmitt 1 participant 7 messages Dec 20, 2007

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover zfcp fixes for scsi-misc

James,

here are some zfcp fixes. They apply on the current version of scsi-misc.

Regards,

Christof Schmitt

#1 lore raw

PATCH 1/6 zfcp: fix use after free bug.

From: Heiko Carstens <[email protected]>

zfcp_erp_strategy_check_fsfreq() checks if it is safe to access the fsf_req associated with the erp_action that gets passed. To test if it is safe it accesses the fsf_req in order to get its index into the hash list. This is broken since the fsf_req might be freed already and the read index has no meaning. It could lead to memory corruption. Fix this by introducing a new zfcp_reqlist_find_safe() method which just checks if addresses are equal. This is slower, but only gets called in case of error recovery.

---

 drivers/s390/scsi/zfcp_def.h |   14 ++++++++++++++
 drivers/s390/scsi/zfcp_erp.c |    3 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)
drivers/s390/scsi/zfcp_def.h +14 -0
@@ -1123,6 +1123,20 @@ zfcp_reqlist_find(struct zfcp_adapter *a
 	return NULL;
 }
 
+static inline struct zfcp_fsf_req *
+zfcp_reqlist_find_safe(struct zfcp_adapter *adapter, struct zfcp_fsf_req *req)
+{
+	struct zfcp_fsf_req *request;
+	unsigned int idx;
+
+	for (idx = 0; idx < REQUEST_LIST_SIZE; idx++) {
+		list_for_each_entry(request, &adapter->req_list[idx], list)
+			if (request == req)
+				return request;
+	}
+	return NULL;
+}
+
 /*
  *  functions needed for reference/usage counting
  */
drivers/s390/scsi/zfcp_erp.c +2 -1
@@ -846,7 +846,8 @@ zfcp_erp_strategy_check_fsfreq(struct zf
 	if (erp_action->fsf_req) {
 		/* take lock to ensure that request is not deleted meanwhile */
 		spin_lock(&adapter->req_list_lock);
-		if (zfcp_reqlist_find(adapter, erp_action->fsf_req->req_id)) {
+		if (zfcp_reqlist_find_safe(adapter, erp_action->fsf_req) &&
+		    erp_action->fsf_req->erp_action == erp_action) {
 			/* fsf_req still exists */
 			debug_text_event(adapter->erp_dbf, 3, "a_ca_req");
 			debug_event(adapter->erp_dbf, 3, &erp_action->fsf_req,
#2 lore raw

PATCH 2/6 zfcp: Fix evaluation of port handles in abort handler

In reply to Christof Schmitt

From: Christof Schmitt <[email protected]>

According to the FSF spec, word 0 (bytes 0-3) has the handle specified with the abort command and word 1 (bytes 4-7) has the handle for the command to be aborted. Fix the if statements that try to compare those.

---

 drivers/s390/scsi/zfcp_fsf.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/s390/scsi/zfcp_fsf.c +4 -5
@@ -1164,8 +1164,8 @@ zfcp_fsf_abort_fcp_command_handler(struc
 {
 	int retval = -EINVAL;
 	struct zfcp_unit *unit;
-	unsigned char status_qual =
-	    new_fsf_req->qtcb->header.fsf_status_qual.word[0];
+	union fsf_status_qual *fsf_stat_qual =
+		&new_fsf_req->qtcb->header.fsf_status_qual;
 
 	if (new_fsf_req->status & ZFCP_STATUS_FSFREQ_ERROR) {
 		/* do not set ZFCP_STATUS_FSFREQ_ABORTSUCCEEDED */
@@ -1178,7 +1178,7 @@ zfcp_fsf_abort_fcp_command_handler(struc
 	switch (new_fsf_req->qtcb->header.fsf_status) {
 
 	case FSF_PORT_HANDLE_NOT_VALID:
-		if (status_qual >> 4 != status_qual % 0xf) {
+		if (fsf_stat_qual->word[0] != fsf_stat_qual->word[1]) {
 			debug_text_event(new_fsf_req->adapter->erp_dbf, 3,
 					 "fsf_s_phand_nv0");
 			/*
@@ -1207,8 +1207,7 @@ zfcp_fsf_abort_fcp_command_handler(struc
 		break;
 
 	case FSF_LUN_HANDLE_NOT_VALID:
-		if (status_qual >> 4 != status_qual % 0xf) {
-			/* 2 */
+		if (fsf_stat_qual->word[0] != fsf_stat_qual->word[1]) {
 			debug_text_event(new_fsf_req->adapter->erp_dbf, 3,
 					 "fsf_s_lhand_nv0");
 			/*
#3 lore raw

PATCH 3/6 zfcp: Hold queue lock when checking port/unit handle for abort command

In reply to Christof Schmitt

From: Christof Schmitt <[email protected]>

We need to hold the queue-lock when checking whether we still have a valid unit/port handle for the abort command, i.e whether we can issue this request for this unit/port. If the error recovery is about to close this unit/port, then it competes for the queue-lock. If the close request issued by the error recovery wins, then it is guaranteed that this unit/port has been blocked for other requests.

---

 drivers/s390/scsi/zfcp_fsf.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)
drivers/s390/scsi/zfcp_fsf.c +8 -13
@@ -1116,6 +1116,10 @@ zfcp_fsf_abort_fcp_command(unsigned long
 		goto out;
 	}
 
+	if (unlikely(!atomic_test_mask(ZFCP_STATUS_COMMON_UNBLOCKED,
+			&unit->status)))
+		goto unit_blocked;
+
 	sbale = zfcp_qdio_sbale_req(fsf_req, fsf_req->sbal_curr, 0);
         sbale[0].flags |= SBAL_FLAGS0_TYPE_READ;
         sbale[1].flags |= SBAL_FLAGS_LAST_ENTRY;
@@ -1131,22 +1135,13 @@ zfcp_fsf_abort_fcp_command(unsigned long
 
 	zfcp_fsf_start_timer(fsf_req, ZFCP_SCSI_ER_TIMEOUT);
 	retval = zfcp_fsf_req_send(fsf_req);
-	if (retval) {
-		ZFCP_LOG_INFO("error: Failed to send abort command request "
-			      "on adapter %s, port 0x%016Lx, unit 0x%016Lx\n",
-			      zfcp_get_busid_by_adapter(adapter),
-			      unit->port->wwpn, unit->fcp_lun);
+	if (!retval)
+		goto out;
+
+ unit_blocked:
 		zfcp_fsf_req_free(fsf_req);
 		fsf_req = NULL;
-		goto out;
-	}
 
-	ZFCP_LOG_DEBUG("Abort FCP Command request initiated "
-		       "(adapter%s, port d_id=0x%06x, "
-		       "unit x%016Lx, old_req_id=0x%lx)\n",
-		       zfcp_get_busid_by_adapter(adapter),
-		       unit->port->d_id,
-		       unit->fcp_lun, old_req_id);
  out:
 	write_unlock_irqrestore(&adapter->request_queue.queue_lock, lock_flags);
 	return fsf_req;
#4 lore raw

PATCH 4/6 zfcp: Hold queue lock when checking port handle for ELS command

In reply to Christof Schmitt

From: Christof Schmitt <[email protected]>

We need to hold the queue-lock when checking whether we still have a valid port handle for the ELS command, i.e whether we can issue this request for this port. If the error recovery is about to close this port, then it competes for the queue-lock. If the close request issued by the error recovery wins, then it is guaranteed that this port has been blocked for other requests.

---

 drivers/s390/scsi/zfcp_erp.c |    2 +-
 drivers/s390/scsi/zfcp_fsf.c |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)
drivers/s390/scsi/zfcp_erp.c +1 -1
@@ -456,7 +456,7 @@ zfcp_test_link(struct zfcp_port *port)
 
 	zfcp_port_get(port);
 	retval = zfcp_erp_adisc(port);
-	if (retval != 0) {
+	if (retval != 0 && retval != -EBUSY) {
 		zfcp_port_put(port);
 		ZFCP_LOG_NORMAL("reopen needed for port 0x%016Lx "
 				"on adapter %s\n ", port->wwpn,
drivers/s390/scsi/zfcp_fsf.c +7 -0
@@ -1668,6 +1668,12 @@ zfcp_fsf_send_els(struct zfcp_send_els *
                 goto failed_req;
 	}
 
+	if (unlikely(!atomic_test_mask(ZFCP_STATUS_COMMON_UNBLOCKED,
+			&els->port->status))) {
+		ret = -EBUSY;
+		goto port_blocked;
+	}
+
 	sbale = zfcp_qdio_sbale_req(fsf_req, fsf_req->sbal_curr, 0);
         if (zfcp_use_one_sbal(els->req, els->req_count,
                               els->resp, els->resp_count)){
@@ -1749,6 +1755,7 @@ zfcp_fsf_send_els(struct zfcp_send_els *
 		       "0x%06x)\n", zfcp_get_busid_by_adapter(adapter), d_id);
 	goto out;
 
+ port_blocked:
  failed_send:
 	zfcp_fsf_req_free(fsf_req);
 
#5 lore raw

PATCH 5/6 zfcp: Hold queue lock when checking port/unit handle for FCP command

In reply to Christof Schmitt

From: Christof Schmitt <[email protected]>

We need to hold the queue-lock when checking whether we still have a valid unit/port handle for the FCP command, i.e whether we can issue this request for this unit/port. If the error recovery is about to close this unit/port, then it competes for the queue-lock. If the close request issued by the error recovery wins, then it is guaranteed that this unit/port has been blocked for other requests.

---

 drivers/s390/scsi/zfcp_fsf.c  |    7 +++++++
 drivers/s390/scsi/zfcp_scsi.c |    8 +++-----
 2 files changed, 10 insertions(+), 5 deletions(-)
drivers/s390/scsi/zfcp_fsf.c +7 -0
@@ -3593,6 +3593,12 @@ zfcp_fsf_send_fcp_command_task(struct zf
 		goto failed_req_create;
 	}
 
+	if (unlikely(!atomic_test_mask(ZFCP_STATUS_COMMON_UNBLOCKED,
+			&unit->status))) {
+		retval = -EBUSY;
+		goto unit_blocked;
+	}
+
 	zfcp_unit_get(unit);
 	fsf_req->unit = unit;
 
@@ -3733,6 +3739,7 @@ zfcp_fsf_send_fcp_command_task(struct zf
  send_failed:
  no_fit:
  failed_scsi_cmnd:
+ unit_blocked:
 	zfcp_unit_put(unit);
 	zfcp_fsf_req_free(fsf_req);
 	fsf_req = NULL;
drivers/s390/scsi/zfcp_scsi.c +3 -5
@@ -258,8 +258,9 @@ zfcp_scsi_command_async(struct zfcp_adap
 		goto out;
 	}
 
-	if (unlikely(
-	     !atomic_test_mask(ZFCP_STATUS_COMMON_UNBLOCKED, &unit->status))) {
+	tmp = zfcp_fsf_send_fcp_command_task(adapter, unit, scpnt, use_timer,
+					     ZFCP_REQ_AUTO_CLEANUP);
+	if (unlikely(tmp == -EBUSY)) {
 		ZFCP_LOG_DEBUG("adapter %s not ready or unit 0x%016Lx "
 			       "on port 0x%016Lx in recovery\n",
 			       zfcp_get_busid_by_unit(unit),
@@ -268,9 +269,6 @@ zfcp_scsi_command_async(struct zfcp_adap
 		goto out;
 	}
 
-	tmp = zfcp_fsf_send_fcp_command_task(adapter, unit, scpnt, use_timer,
-					     ZFCP_REQ_AUTO_CLEANUP);
-
 	if (unlikely(tmp < 0)) {
 		ZFCP_LOG_DEBUG("error: initiation of Send FCP Cmnd failed\n");
 		retval = SCSI_MLQUEUE_HOST_BUSY;
#6 lore raw

PATCH 6/6 zfcp: Hold queue lock when checking port/unit handle for task management cmd

In reply to Christof Schmitt

From: Christof Schmitt <[email protected]>

We need to hold the queue-lock when checking whether we still have a valid unit/port handle for the task management command, i.e whether we can issue this request for this unit/port. If the error recovery is about to close this unit/port, then it competes for the queue-lock. If the close request issued by the error recovery wins, then it is guaranteed that this unit/port has been blocked for other requests.

---

 drivers/s390/scsi/zfcp_fsf.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)
drivers/s390/scsi/zfcp_fsf.c +9 -17
@@ -3774,6 +3774,10 @@ zfcp_fsf_send_fcp_command_task_managemen
 		goto out;
 	}
 
+	if (unlikely(!atomic_test_mask(ZFCP_STATUS_COMMON_UNBLOCKED,
+			&unit->status)))
+		goto unit_blocked;
+
 	/*
 	 * Used to decide on proper handler in the return path,
 	 * could be either zfcp_fsf_send_fcp_command_task_handler or
@@ -3807,25 +3811,13 @@ zfcp_fsf_send_fcp_command_task_managemen
 
 	zfcp_fsf_start_timer(fsf_req, ZFCP_SCSI_ER_TIMEOUT);
 	retval = zfcp_fsf_req_send(fsf_req);
-	if (retval) {
-		ZFCP_LOG_INFO("error: Could not send an FCP-command (task "
-			      "management) on adapter %s, port 0x%016Lx for "
-			      "unit LUN 0x%016Lx\n",
-			      zfcp_get_busid_by_adapter(adapter),
-			      unit->port->wwpn,
-			      unit->fcp_lun);
-		zfcp_fsf_req_free(fsf_req);
-		fsf_req = NULL;
+	if (!retval)
 		goto out;
-	}
 
-	ZFCP_LOG_TRACE("Send FCP Command (task management function) initiated "
-		       "(adapter %s, port 0x%016Lx, unit 0x%016Lx, "
-		       "tm_flags=0x%x)\n",
-		       zfcp_get_busid_by_adapter(adapter),
-		       unit->port->wwpn,
-		       unit->fcp_lun,
-		       tm_flags);
+ unit_blocked:
+	zfcp_fsf_req_free(fsf_req);
+	fsf_req = NULL;
+
  out:
 	write_unlock_irqrestore(&adapter->request_queue.queue_lock, lock_flags);
 	return fsf_req;