[patch 0/6] zfcp fixes for scsi-misc

[patch 0/6] zfcp fixes for scsi-misc

[Christof Schmitt]

James,

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

Regards,

Christof Schmitt

[patch 1/6] zfcp: fix use after free bug.

[Christof Schmitt]

[-- Attachment #1: 807-zfcp-kfree.diff --]
[-- Type: text/plain, Size: 2253 bytes --]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

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.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_def.h |   14 ++++++++++++++
 drivers/s390/scsi/zfcp_erp.c |    3 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

--- a/drivers/s390/scsi/zfcp_def.h	2007-12-20 11:15:10.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_def.h	2007-12-20 11:17:46.000000000 +0100
@@ -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
  */
--- a/drivers/s390/scsi/zfcp_erp.c	2007-12-20 11:15:10.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_erp.c	2007-12-20 11:17:46.000000000 +0100
@@ -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,

-- 

[patch 2/6] zfcp: Fix evaluation of port handles in abort handler

[Christof Schmitt]

[-- Attachment #1: 808-zfcp-handles.diff --]
[-- Type: text/plain, Size: 1689 bytes --]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

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.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_fsf.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

--- a/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:16:38.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:03.000000000 +0100
@@ -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");
 			/*

-- 

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

[Christof Schmitt]

[-- Attachment #1: 809-zfcp-lock1.diff --]
[-- Type: text/plain, Size: 2044 bytes --]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

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.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_fsf.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

--- a/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:03.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:23.000000000 +0100
@@ -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;

-- 

[patch 4/6] zfcp: Hold queue lock when checking port handle for ELS command

[Christof Schmitt]

[-- Attachment #1: 810-zfcp-lock2.diff --]
[-- Type: text/plain, Size: 1893 bytes --]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

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.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_erp.c |    2 +-
 drivers/s390/scsi/zfcp_fsf.c |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

--- a/drivers/s390/scsi/zfcp_erp.c	2007-12-20 11:17:46.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_erp.c	2007-12-20 11:18:42.000000000 +0100
@@ -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,
--- a/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:23.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:42.000000000 +0100
@@ -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);
 

-- 

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

[Christof Schmitt]

[-- Attachment #1: 811-zfcp-lock3.diff --]
[-- Type: text/plain, Size: 2252 bytes --]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

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.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_fsf.c  |    7 +++++++
 drivers/s390/scsi/zfcp_scsi.c |    8 +++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

--- a/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:18:42.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:19:01.000000000 +0100
@@ -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;
--- a/drivers/s390/scsi/zfcp_scsi.c	2007-12-20 11:15:10.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_scsi.c	2007-12-20 11:19:01.000000000 +0100
@@ -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;

-- 

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

[Christof Schmitt]

[-- Attachment #1: 812-zfcp-lock4.diff --]
[-- Type: text/plain, Size: 2120 bytes --]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

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.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 drivers/s390/scsi/zfcp_fsf.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

--- a/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:19:01.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2007-12-20 11:19:19.000000000 +0100
@@ -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;

--