All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9 V5] iscsi fixes and cleanups
@ 2021-02-03  1:33 Mike Christie
  2021-02-03  1:33 ` [PATCH 1/9] libiscsi: fix iscsi_prep_scsi_cmd_pdu error handling Mike Christie
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao

The following patches made over Martin's 5.12 branches contain
fixes for a cmd lifetime bug, software iscsi can_queue setup,
and a couple of the lock cleanup patches Lee has already ackd.

V5:
- Fix up KERN_ERR/INFO use when detecting invalid max_cmds values
from the user.
- Set iscsi_tcp can queue to max value it can support not including
mgmt cmds since the driver itself is not limited and that is a libiscsi
layer limit.
- Added the iscsi session class lock cleanup from the lock cleanup
patchset since it was reviewed already and this is now a patchset
for the next feature window.

V4:
- Add patch:
[PATCH 4/7] libiscsi: fix iscsi host workq destruction
to fix an issue where the user might only call iscsi_host_alloc then
iscsi_host_free and that was leaving the iscsi workqueue running.
- Add check for if a driver were to set can_queue to ISCSI_MGMT_CMDS_MAX
or less.
V3:
- Add some patches for issues found while testing.
	- session queue depth was stuck at 128
	- cmd window could not be detected when session was relogged in.
- Patch "libiscsi: drop taskqueuelock" had a bug where we did not
disable bhs and during xmit thread suspension leaked the current task.
V2:
- Take back_lock when looping over running cmds in iscsi_eh_cmd_timed_out
in case those complete while we are accessing them.




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/9] libiscsi: fix iscsi_prep_scsi_cmd_pdu error handling
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

If iscsi_prep_scsi_cmd_pdu fails we try to add it back to the
cmdqueue, but we leave it partially setup. We don't have functions
that can undo the pdu and init task setup. We only have cleanup_task
which can cleanup both parts. So this has us just fail the cmd and
go through the standard cleanup routine and then have scsi-ml retry
it like is done when it fails in the queuecommand path.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/libiscsi.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 4e668aafbcca..cee1dbaa1b93 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1532,14 +1532,9 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 		}
 		rc = iscsi_prep_scsi_cmd_pdu(conn->task);
 		if (rc) {
-			if (rc == -ENOMEM || rc == -EACCES) {
-				spin_lock_bh(&conn->taskqueuelock);
-				list_add_tail(&conn->task->running,
-					      &conn->cmdqueue);
-				conn->task = NULL;
-				spin_unlock_bh(&conn->taskqueuelock);
-				goto done;
-			} else
+			if (rc == -ENOMEM || rc == -EACCES)
+				fail_scsi_task(conn->task, DID_IMM_RETRY);
+			else
 				fail_scsi_task(conn->task, DID_ABORT);
 			spin_lock_bh(&conn->taskqueuelock);
 			continue;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/9] libiscsi: drop taskqueuelock
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
  2021-02-03  1:33 ` [PATCH 1/9] libiscsi: fix iscsi_prep_scsi_cmd_pdu error handling Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03 10:19     ` Dan Carpenter
  2021-02-03  1:33 ` [PATCH 3/9] libiscsi: fix iscsi_task use after free Mike Christie
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

The purpose of the taskqueuelock was to handle the issue where a bad
target decides to send a R2T and before it's data has been sent
decides to send a cmd response to complete the cmd. The following
patches fix up the frwd/back locks so they are taken from the
queue/xmit (frwd) and completion (back) paths again. To get there
this patch removes the taskqueuelock which for iscsi xmit wq based
drivers was taken in the queue, xmit and completion paths.

Instead of the lock, we just make sure we have a ref to the task
when we queue a R2T, and then we always remove the task from the
requeue list in the xmit path or the forced cleanup paths.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/libiscsi.c     | 178 +++++++++++++++++++++++-------------
 drivers/scsi/libiscsi_tcp.c |  84 +++++++++++------
 include/scsi/libiscsi.h     |   4 +-
 3 files changed, 171 insertions(+), 95 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index cee1dbaa1b93..3d74fdd9f31a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -523,16 +523,6 @@ static void iscsi_complete_task(struct iscsi_task *task, int state)
 	WARN_ON_ONCE(task->state == ISCSI_TASK_FREE);
 	task->state = state;
 
-	spin_lock_bh(&conn->taskqueuelock);
-	if (!list_empty(&task->running)) {
-		pr_debug_once("%s while task on list", __func__);
-		list_del_init(&task->running);
-	}
-	spin_unlock_bh(&conn->taskqueuelock);
-
-	if (conn->task == task)
-		conn->task = NULL;
-
 	if (READ_ONCE(conn->ping_task) == task)
 		WRITE_ONCE(conn->ping_task, NULL);
 
@@ -564,9 +554,40 @@ void iscsi_complete_scsi_task(struct iscsi_task *task,
 }
 EXPORT_SYMBOL_GPL(iscsi_complete_scsi_task);
 
+/*
+ * Must be called with back and frwd lock
+ */
+static bool cleanup_queued_task(struct iscsi_task *task)
+{
+	struct iscsi_conn *conn = task->conn;
+	bool early_complete = false;
+
+	/* Bad target might have completed task while it was still running */
+	if (task->state == ISCSI_TASK_COMPLETED)
+		early_complete = true;
+
+	if (!list_empty(&task->running)) {
+		list_del_init(&task->running);
+		/*
+		 * If it's on a list but still running, this could be from
+		 * a bad target sending a rsp early, cleanup from a TMF, or
+		 * session recovery.
+		 */
+		if (task->state == ISCSI_TASK_RUNNING ||
+		    task->state == ISCSI_TASK_COMPLETED)
+			__iscsi_put_task(task);
+	}
+
+	if (conn->task == task) {
+		conn->task = NULL;
+		__iscsi_put_task(task);
+	}
+
+	return early_complete;
+}
 
 /*
- * session back_lock must be held and if not called for a task that is
+ * session frwd_lock must be held and if not called for a task that is
  * still pending or from the xmit thread, then xmit thread must
  * be suspended.
  */
@@ -585,6 +606,13 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 	if (!sc)
 		return;
 
+	/* regular RX path uses back_lock */
+	spin_lock_bh(&conn->session->back_lock);
+	if (cleanup_queued_task(task)) {
+		spin_unlock_bh(&conn->session->back_lock);
+		return;
+	}
+
 	if (task->state == ISCSI_TASK_PENDING) {
 		/*
 		 * cmd never made it to the xmit thread, so we should not count
@@ -600,9 +628,6 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 
 	sc->result = err << 16;
 	scsi_set_resid(sc, scsi_bufflen(sc));
-
-	/* regular RX path uses back_lock */
-	spin_lock_bh(&conn->session->back_lock);
 	iscsi_complete_task(task, state);
 	spin_unlock_bh(&conn->session->back_lock);
 }
@@ -748,9 +773,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 		if (session->tt->xmit_task(task))
 			goto free_task;
 	} else {
-		spin_lock_bh(&conn->taskqueuelock);
 		list_add_tail(&task->running, &conn->mgmtqueue);
-		spin_unlock_bh(&conn->taskqueuelock);
 		iscsi_conn_queue_work(conn);
 	}
 
@@ -1411,31 +1434,61 @@ static int iscsi_check_cmdsn_window_closed(struct iscsi_conn *conn)
 	return 0;
 }
 
-static int iscsi_xmit_task(struct iscsi_conn *conn)
+static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
+			   bool was_requeue)
 {
-	struct iscsi_task *task = conn->task;
 	int rc;
 
-	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
-		return -ENODATA;
-
 	spin_lock_bh(&conn->session->back_lock);
-	if (conn->task == NULL) {
+
+	if (!conn->task) {
+		/* Take a ref so we can access it after xmit_task() */
+		__iscsi_get_task(task);
+	} else {
+		/* Already have a ref from when we failed to send it last call */
+		conn->task = NULL;
+	}
+
+	/*
+	 * If this was a requeue for a R2T we have an extra ref on the task in
+	 * case a bad target sends a cmd rsp before we have handled the task.
+	 */
+	if (was_requeue)
+		__iscsi_put_task(task);
+
+	/*
+	 * Do this after dropping the extra ref because if this was a requeue
+	 * it's removed from that list and cleanup_queued_task would miss it.
+	 */
+	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
+		/*
+		 * Save the task and ref in case we weren't cleaning up this
+		 * task and get woken up again.
+		 */
+		conn->task = task;
 		spin_unlock_bh(&conn->session->back_lock);
 		return -ENODATA;
 	}
-	__iscsi_get_task(task);
 	spin_unlock_bh(&conn->session->back_lock);
+
 	spin_unlock_bh(&conn->session->frwd_lock);
 	rc = conn->session->tt->xmit_task(task);
 	spin_lock_bh(&conn->session->frwd_lock);
 	if (!rc) {
 		/* done with this task */
 		task->last_xfer = jiffies;
-		conn->task = NULL;
 	}
 	/* regular RX path uses back_lock */
 	spin_lock(&conn->session->back_lock);
+	if (rc && task->state == ISCSI_TASK_RUNNING) {
+		/*
+		 * get an extra ref that is released next time we access it
+		 * as conn->task above.
+		 */
+		__iscsi_get_task(task);
+		conn->task = task;
+	}
+
 	__iscsi_put_task(task);
 	spin_unlock(&conn->session->back_lock);
 	return rc;
@@ -1445,9 +1498,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
  * iscsi_requeue_task - requeue task to run from session workqueue
  * @task: task to requeue
  *
- * LLDs that need to run a task from the session workqueue should call
- * this. The session frwd_lock must be held. This should only be called
- * by software drivers.
+ * Callers must have taken a ref to the task that is going to be requeued.
  */
 void iscsi_requeue_task(struct iscsi_task *task)
 {
@@ -1457,11 +1508,18 @@ void iscsi_requeue_task(struct iscsi_task *task)
 	 * this may be on the requeue list already if the xmit_task callout
 	 * is handling the r2ts while we are adding new ones
 	 */
-	spin_lock_bh(&conn->taskqueuelock);
-	if (list_empty(&task->running))
+	spin_lock_bh(&conn->session->frwd_lock);
+	if (list_empty(&task->running)) {
 		list_add_tail(&task->running, &conn->requeue);
-	spin_unlock_bh(&conn->taskqueuelock);
+	} else {
+		/*
+		 * Don't need the extra ref since it's already requeued and
+		 * has a ref.
+		 */
+		iscsi_put_task(task);
+	}
 	iscsi_conn_queue_work(conn);
+	spin_unlock_bh(&conn->session->frwd_lock);
 }
 EXPORT_SYMBOL_GPL(iscsi_requeue_task);
 
@@ -1487,7 +1545,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 	}
 
 	if (conn->task) {
-		rc = iscsi_xmit_task(conn);
+		rc = iscsi_xmit_task(conn, conn->task, false);
 	        if (rc)
 		        goto done;
 	}
@@ -1497,49 +1555,41 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 	 * only have one nop-out as a ping from us and targets should not
 	 * overflow us with nop-ins
 	 */
-	spin_lock_bh(&conn->taskqueuelock);
 check_mgmt:
 	while (!list_empty(&conn->mgmtqueue)) {
-		conn->task = list_entry(conn->mgmtqueue.next,
-					 struct iscsi_task, running);
-		list_del_init(&conn->task->running);
-		spin_unlock_bh(&conn->taskqueuelock);
-		if (iscsi_prep_mgmt_task(conn, conn->task)) {
+		task = list_entry(conn->mgmtqueue.next, struct iscsi_task,
+				  running);
+		list_del_init(&task->running);
+		if (iscsi_prep_mgmt_task(conn, task)) {
 			/* regular RX path uses back_lock */
 			spin_lock_bh(&conn->session->back_lock);
-			__iscsi_put_task(conn->task);
+			__iscsi_put_task(task);
 			spin_unlock_bh(&conn->session->back_lock);
-			conn->task = NULL;
-			spin_lock_bh(&conn->taskqueuelock);
 			continue;
 		}
-		rc = iscsi_xmit_task(conn);
+		rc = iscsi_xmit_task(conn, task, false);
 		if (rc)
 			goto done;
-		spin_lock_bh(&conn->taskqueuelock);
 	}
 
 	/* process pending command queue */
 	while (!list_empty(&conn->cmdqueue)) {
-		conn->task = list_entry(conn->cmdqueue.next, struct iscsi_task,
-					running);
-		list_del_init(&conn->task->running);
-		spin_unlock_bh(&conn->taskqueuelock);
+		task = list_entry(conn->cmdqueue.next, struct iscsi_task,
+				  running);
+		list_del_init(&task->running);
 		if (conn->session->state == ISCSI_STATE_LOGGING_OUT) {
-			fail_scsi_task(conn->task, DID_IMM_RETRY);
-			spin_lock_bh(&conn->taskqueuelock);
+			fail_scsi_task(task, DID_IMM_RETRY);
 			continue;
 		}
-		rc = iscsi_prep_scsi_cmd_pdu(conn->task);
+		rc = iscsi_prep_scsi_cmd_pdu(task);
 		if (rc) {
 			if (rc == -ENOMEM || rc == -EACCES)
-				fail_scsi_task(conn->task, DID_IMM_RETRY);
+				fail_scsi_task(task, DID_IMM_RETRY);
 			else
-				fail_scsi_task(conn->task, DID_ABORT);
-			spin_lock_bh(&conn->taskqueuelock);
+				fail_scsi_task(task, DID_ABORT);
 			continue;
 		}
-		rc = iscsi_xmit_task(conn);
+		rc = iscsi_xmit_task(conn, task, false);
 		if (rc)
 			goto done;
 		/*
@@ -1547,7 +1597,6 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 		 * we need to check the mgmt queue for nops that need to
 		 * be sent to aviod starvation
 		 */
-		spin_lock_bh(&conn->taskqueuelock);
 		if (!list_empty(&conn->mgmtqueue))
 			goto check_mgmt;
 	}
@@ -1561,21 +1610,17 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 
 		task = list_entry(conn->requeue.next, struct iscsi_task,
 				  running);
+
 		if (iscsi_check_tmf_restrictions(task, ISCSI_OP_SCSI_DATA_OUT))
 			break;
 
-		conn->task = task;
-		list_del_init(&conn->task->running);
-		conn->task->state = ISCSI_TASK_RUNNING;
-		spin_unlock_bh(&conn->taskqueuelock);
-		rc = iscsi_xmit_task(conn);
+		list_del_init(&task->running);
+		rc = iscsi_xmit_task(conn, task, true);
 		if (rc)
 			goto done;
-		spin_lock_bh(&conn->taskqueuelock);
 		if (!list_empty(&conn->mgmtqueue))
 			goto check_mgmt;
 	}
-	spin_unlock_bh(&conn->taskqueuelock);
 	spin_unlock_bh(&conn->session->frwd_lock);
 	return -ENODATA;
 
@@ -1741,9 +1786,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
 			goto prepd_reject;
 		}
 	} else {
-		spin_lock_bh(&conn->taskqueuelock);
 		list_add_tail(&task->running, &conn->cmdqueue);
-		spin_unlock_bh(&conn->taskqueuelock);
 		iscsi_conn_queue_work(conn);
 	}
 
@@ -2914,7 +2957,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
 	INIT_LIST_HEAD(&conn->mgmtqueue);
 	INIT_LIST_HEAD(&conn->cmdqueue);
 	INIT_LIST_HEAD(&conn->requeue);
-	spin_lock_init(&conn->taskqueuelock);
 	INIT_WORK(&conn->xmitwork, iscsi_xmitworker);
 
 	/* allocate login_task used for the login/text sequences */
@@ -3080,10 +3122,16 @@ fail_mgmt_tasks(struct iscsi_session *session, struct iscsi_conn *conn)
 		ISCSI_DBG_SESSION(conn->session,
 				  "failing mgmt itt 0x%x state %d\n",
 				  task->itt, task->state);
+
+		spin_lock_bh(&session->back_lock);
+		if (cleanup_queued_task(task)) {
+			spin_unlock_bh(&session->back_lock);
+			continue;
+		}
+
 		state = ISCSI_TASK_ABRT_SESS_RECOV;
 		if (task->state == ISCSI_TASK_PENDING)
 			state = ISCSI_TASK_COMPLETED;
-		spin_lock_bh(&session->back_lock);
 		iscsi_complete_task(task, state);
 		spin_unlock_bh(&session->back_lock);
 	}
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 83f14b2c8804..14c9169f13ec 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -524,48 +524,79 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task)
 /**
  * iscsi_tcp_r2t_rsp - iSCSI R2T Response processing
  * @conn: iscsi connection
- * @task: scsi command task
+ * @hdr: PDU header
  */
-static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
+static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 {
 	struct iscsi_session *session = conn->session;
-	struct iscsi_tcp_task *tcp_task = task->dd_data;
-	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
-	struct iscsi_r2t_rsp *rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
+	struct iscsi_tcp_task *tcp_task;
+	struct iscsi_tcp_conn *tcp_conn;
+	struct iscsi_r2t_rsp *rhdr;
 	struct iscsi_r2t_info *r2t;
-	int r2tsn = be32_to_cpu(rhdr->r2tsn);
+	struct iscsi_task *task;
 	u32 data_length;
 	u32 data_offset;
+	int r2tsn;
 	int rc;
 
+	spin_lock(&session->back_lock);
+	task = iscsi_itt_to_ctask(conn, hdr->itt);
+	if (!task) {
+		spin_unlock(&session->back_lock);
+		return ISCSI_ERR_BAD_ITT;
+	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
+		spin_unlock(&session->back_lock);
+		return ISCSI_ERR_PROTO;
+	}
+	/*
+	 * A bad target might complete the cmd before we have handled R2Ts
+	 * so get a ref to the task that will be dropped in the xmit path.
+	 */
+	if (task->state != ISCSI_TASK_RUNNING) {
+		spin_unlock(&session->back_lock);
+		/* Let the path that got the early rsp complete it */
+		return 0;
+	}
+	task->last_xfer = jiffies;
+	__iscsi_get_task(task);
+
+	tcp_conn = conn->dd_data;
+	rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
+	/* fill-in new R2T associated with the task */
+	iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
+	spin_unlock(&session->back_lock);
+
 	if (tcp_conn->in.datalen) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "invalid R2t with datalen %d\n",
 				  tcp_conn->in.datalen);
-		return ISCSI_ERR_DATALEN;
+		rc = ISCSI_ERR_DATALEN;
+		goto put_task;
 	}
 
+	tcp_task = task->dd_data;
+	r2tsn = be32_to_cpu(rhdr->r2tsn);
 	if (tcp_task->exp_datasn != r2tsn){
 		ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n",
 			      tcp_task->exp_datasn, r2tsn);
-		return ISCSI_ERR_R2TSN;
+		rc = ISCSI_ERR_R2TSN;
+		goto put_task;
 	}
 
-	/* fill-in new R2T associated with the task */
-	iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr);
-
 	if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) {
 		iscsi_conn_printk(KERN_INFO, conn,
 				  "dropping R2T itt %d in recovery.\n",
 				  task->itt);
-		return 0;
+		rc = 0;
+		goto put_task;
 	}
 
 	data_length = be32_to_cpu(rhdr->data_length);
 	if (data_length == 0) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "invalid R2T with zero data len\n");
-		return ISCSI_ERR_DATALEN;
+		rc = ISCSI_ERR_DATALEN;
+		goto put_task;
 	}
 
 	if (data_length > session->max_burst)
@@ -579,7 +610,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
 				  "invalid R2T with data len %u at offset %u "
 				  "and total length %d\n", data_length,
 				  data_offset, task->sc->sdb.length);
-		return ISCSI_ERR_DATALEN;
+		rc = ISCSI_ERR_DATALEN;
+		goto put_task;
 	}
 
 	spin_lock(&tcp_task->pool2queue);
@@ -589,7 +621,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
 				  "Target has sent more R2Ts than it "
 				  "negotiated for or driver has leaked.\n");
 		spin_unlock(&tcp_task->pool2queue);
-		return ISCSI_ERR_PROTO;
+		rc = ISCSI_ERR_PROTO;
+		goto put_task;
 	}
 
 	r2t->exp_statsn = rhdr->statsn;
@@ -607,6 +640,10 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task)
 
 	iscsi_requeue_task(task);
 	return 0;
+
+put_task:
+	iscsi_put_task(task);
+	return rc;
 }
 
 /*
@@ -730,20 +767,11 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 		rc = iscsi_complete_pdu(conn, hdr, NULL, 0);
 		break;
 	case ISCSI_OP_R2T:
-		spin_lock(&conn->session->back_lock);
-		task = iscsi_itt_to_ctask(conn, hdr->itt);
-		spin_unlock(&conn->session->back_lock);
-		if (!task)
-			rc = ISCSI_ERR_BAD_ITT;
-		else if (ahslen)
+		if (ahslen) {
 			rc = ISCSI_ERR_AHSLEN;
-		else if (task->sc->sc_data_direction == DMA_TO_DEVICE) {
-			task->last_xfer = jiffies;
-			spin_lock(&conn->session->frwd_lock);
-			rc = iscsi_tcp_r2t_rsp(conn, task);
-			spin_unlock(&conn->session->frwd_lock);
-		} else
-			rc = ISCSI_ERR_PROTO;
+			break;
+		}
+		rc = iscsi_tcp_r2t_rsp(conn, hdr);
 		break;
 	case ISCSI_OP_LOGIN_RSP:
 	case ISCSI_OP_TEXT_RSP:
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index b3bbd10eb3f0..44a9554aea62 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -187,7 +187,7 @@ struct iscsi_conn {
 	struct iscsi_task	*task;		/* xmit task in progress */
 
 	/* xmit */
-	spinlock_t		taskqueuelock;  /* protects the next three lists */
+	/* items must be added/deleted under frwd lock */
 	struct list_head	mgmtqueue;	/* mgmt (control) xmit queue */
 	struct list_head	cmdqueue;	/* data-path cmd queue */
 	struct list_head	requeue;	/* tasks needing another run */
@@ -332,7 +332,7 @@ struct iscsi_session {
 						 * cmdsn, queued_cmdsn     *
 						 * session resources:      *
 						 * - cmdpool kfifo_out ,   *
-						 * - mgmtpool,		   */
+						 * - mgmtpool, queues	   */
 	spinlock_t		back_lock;	/* protects cmdsn_exp      *
 						 * cmdsn_max,              *
 						 * cmdpool kfifo_in        */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/9] libiscsi: fix iscsi_task use after free
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
  2021-02-03  1:33 ` [PATCH 1/9] libiscsi: fix iscsi_prep_scsi_cmd_pdu error handling Mike Christie
  2021-02-03  1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 4/9] libiscsi: fix iscsi host workq destruction Mike Christie
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie, Wu Bo

The following bug was reported and debugged by wubo40@huawei.com:

When testing kernel 4.18 version, NULL pointer dereference problem
occurs
in iscsi_eh_cmd_timed_out function.

I think this bug in the upstream is still exists.

The analysis reasons are as follows:
1)  For some reason, I/O command did not complete within
    the timeout period. The block layer timer works,
    call scsi_times_out() to handle I/O timeout logic.
    At the same time the command just completes.

2)  scsi_times_out() call iscsi_eh_cmd_timed_out()
    to processing timeout logic.  although there is an NULL judgment
        for the task, the task has not been released yet now.

3)  iscsi_complete_task() call __iscsi_put_task(),
    The task reference count reaches zero, the conditions for free task
    is met, then iscsi_free_task () free the task,
    and let sc->SCp.ptr = NULL. After iscsi_eh_cmd_timed_out passes
    the task judgment check, there may be NULL dereference scenarios
    later.

   CPU0                                                CPU3

    |- scsi_times_out()                                 |-
iscsi_complete_task()
    |                                                   |
    |- iscsi_eh_cmd_timed_out()                         |-
__iscsi_put_task()
    |                                                   |
    |- task=sc->SCp.ptr, task is not NUL, check passed  |-
iscsi_free_task(task)
    |                                                   |
    |                                                   |-> sc->SCp.ptr
= NULL
    |                                                   |
    |- task is NULL now, NULL pointer dereference       |
    |                                                   |
   \|/                                                 \|/

Calltrace:
[380751.840862] BUG: unable to handle kernel NULL pointer dereference at
0000000000000138
[380751.843709] PGD 0 P4D 0
[380751.844770] Oops: 0000 [#1] SMP PTI
[380751.846283] CPU: 0 PID: 403 Comm: kworker/0:1H Kdump: loaded
Tainted: G
[380751.851467] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
[380751.856521] Workqueue: kblockd blk_mq_timeout_work
[380751.858527] RIP: 0010:iscsi_eh_cmd_timed_out+0x15e/0x2e0 [libiscsi]
[380751.861129] Code: 83 ea 01 48 8d 74 d0 08 48 8b 10 48 8b 4a 50 48 85
c9 74 2c 48 39 d5 74
[380751.868811] RSP: 0018:ffffc1e280a5fd58 EFLAGS: 00010246
[380751.870978] RAX: ffff9fd1e84e15e0 RBX: ffff9fd1e84e6dd0 RCX:
0000000116acc580
[380751.873791] RDX: ffff9fd1f97a9400 RSI: ffff9fd1e84e1800 RDI:
ffff9fd1e4d6d420
[380751.876059] RBP: ffff9fd1e4d49000 R08: 0000000116acc580 R09:
0000000116acc580
[380751.878284] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff9fd1e6e931e8
[380751.880500] R13: ffff9fd1e84e6ee0 R14: 0000000000000010 R15:
0000000000000003
[380751.882687] FS:  0000000000000000(0000) GS:ffff9fd1fac00000(0000)
knlGS:0000000000000000
[380751.885236] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[380751.887059] CR2: 0000000000000138 CR3: 000000011860a001 CR4:
00000000003606f0
[380751.889308] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[380751.891523] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[380751.893738] Call Trace:
[380751.894639]  scsi_times_out+0x60/0x1c0
[380751.895861]  blk_mq_check_expired+0x144/0x200
[380751.897302]  ? __switch_to_asm+0x35/0x70
[380751.898551]  blk_mq_queue_tag_busy_iter+0x195/0x2e0
[380751.900091]  ? __blk_mq_requeue_request+0x100/0x100
[380751.901611]  ? __switch_to_asm+0x41/0x70
[380751.902853]  ? __blk_mq_requeue_request+0x100/0x100
[380751.904398]  blk_mq_timeout_work+0x54/0x130
[380751.905740]  process_one_work+0x195/0x390
[380751.907228]  worker_thread+0x30/0x390
[380751.908713]  ? process_one_work+0x390/0x390
[380751.910350]  kthread+0x10d/0x130
[380751.911470]  ? kthread_flush_work_fn+0x10/0x10
[380751.913007]  ret_from_fork+0x35/0x40

crash> dis -l iscsi_eh_cmd_timed_out+0x15e
xxxxx/drivers/scsi/libiscsi.c: 2062

1970 enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd
*sc)
{
...
1984         spin_lock_bh(&session->frwd_lock);
1985         task = (struct iscsi_task *)sc->SCp.ptr;
1986         if (!task) {
1987                 /*
1988                  * Raced with completion. Blk layer has taken
ownership
1989                  * so let timeout code complete it now.
1990                  */
1991                 rc = BLK_EH_DONE;
1992                 goto done;
1993         }

...

2052         for (i = 0; i < conn->session->cmds_max; i++) {
2053                 running_task = conn->session->cmds[i];
2054                 if (!running_task->sc || running_task == task ||
2055                      running_task->state != ISCSI_TASK_RUNNING)
2056                         continue;
2057
2058                 /*
2059                  * Only check if cmds started before this one have
made
2060                  * progress, or this could never fail
2061                  */
2062                 if (time_after(running_task->sc->jiffies_at_alloc,
2063                                task->sc->jiffies_at_alloc))    <---
2064                         continue;
2065
...
}

carsh> struct scsi_cmnd ffff9fd1e6e931e8
struct scsi_cmnd {
  ...
  SCp = {
    ptr = 0x0,   <--- iscsi_task
    this_residual = 0,
    ...
  },
}

To prevent this, we take a ref to the cmd under the back (completion)
lock so if the completion side were to call iscsi_complete_task on the
task while the timer/eh paths are not holding the back_lock it will
not be freed from under us.

Note that this requires the previous patch, "libiscsi: drop
taskqueuelock" because bnx2i sleeps in its cleanup_task callout if the
cmd is aborted. If the EH/timer and completion path are racing we don't
know which path will do the last put. The previous patch moved the
operations we needed to do under the forward lock to cleanup_queued_task.
Once that has run we can drop the forward lock for the cmd and bnx2i no
longer has to worry about if the EH, timer or completion path did the
ast put and if the forward lock is held or not since it won't be.

Reported-by: Wu Bo <wubo40@huawei.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/bnx2i/bnx2i_iscsi.c |  2 -
 drivers/scsi/libiscsi.c          | 71 ++++++++++++++++++++------------
 2 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index fdd446765311..1e6d8f62ea3c 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1171,10 +1171,8 @@ static void bnx2i_cleanup_task(struct iscsi_task *task)
 		bnx2i_send_cmd_cleanup_req(hba, task->dd_data);
 
 		spin_unlock_bh(&conn->session->back_lock);
-		spin_unlock_bh(&conn->session->frwd_lock);
 		wait_for_completion_timeout(&bnx2i_conn->cmd_cleanup_cmpl,
 				msecs_to_jiffies(ISCSI_CMD_CLEANUP_TIMEOUT));
-		spin_lock_bh(&conn->session->frwd_lock);
 		spin_lock_bh(&conn->session->back_lock);
 	}
 	bnx2i_iscsi_unmap_sg_list(task->dd_data);
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3d74fdd9f31a..ec159bcb7460 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -587,9 +587,8 @@ static bool cleanup_queued_task(struct iscsi_task *task)
 }
 
 /*
- * session frwd_lock must be held and if not called for a task that is
- * still pending or from the xmit thread, then xmit thread must
- * be suspended.
+ * session frwd lock must be held and if not called for a task that is still
+ * pending or from the xmit thread, then xmit thread must be suspended
  */
 static void fail_scsi_task(struct iscsi_task *task, int err)
 {
@@ -597,16 +596,6 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 	struct scsi_cmnd *sc;
 	int state;
 
-	/*
-	 * if a command completes and we get a successful tmf response
-	 * we will hit this because the scsi eh abort code does not take
-	 * a ref to the task.
-	 */
-	sc = task->sc;
-	if (!sc)
-		return;
-
-	/* regular RX path uses back_lock */
 	spin_lock_bh(&conn->session->back_lock);
 	if (cleanup_queued_task(task)) {
 		spin_unlock_bh(&conn->session->back_lock);
@@ -626,6 +615,7 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 	else
 		state = ISCSI_TASK_ABRT_TMF;
 
+	sc = task->sc;
 	sc->result = err << 16;
 	scsi_set_resid(sc, scsi_bufflen(sc));
 	iscsi_complete_task(task, state);
@@ -1893,27 +1883,39 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
 }
 
 /*
- * Fail commands. session lock held and recv side suspended and xmit
- * thread flushed
+ * Fail commands. session frwd lock held and xmit thread flushed.
  */
 static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
 {
+	struct iscsi_session *session = conn->session;
 	struct iscsi_task *task;
 	int i;
 
-	for (i = 0; i < conn->session->cmds_max; i++) {
-		task = conn->session->cmds[i];
+	spin_lock_bh(&session->back_lock);
+	for (i = 0; i < session->cmds_max; i++) {
+		task = session->cmds[i];
 		if (!task->sc || task->state == ISCSI_TASK_FREE)
 			continue;
 
 		if (lun != -1 && lun != task->sc->device->lun)
 			continue;
 
-		ISCSI_DBG_SESSION(conn->session,
+		__iscsi_get_task(task);
+		spin_unlock_bh(&session->back_lock);
+
+		ISCSI_DBG_SESSION(session,
 				  "failing sc %p itt 0x%x state %d\n",
 				  task->sc, task->itt, task->state);
 		fail_scsi_task(task, error);
+
+		spin_unlock_bh(&session->frwd_lock);
+		iscsi_put_task(task);
+		spin_lock_bh(&session->frwd_lock);
+
+		spin_lock_bh(&session->back_lock);
 	}
+
+	spin_unlock_bh(&session->back_lock);
 }
 
 /**
@@ -1991,6 +1993,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 	ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
 
 	spin_lock_bh(&session->frwd_lock);
+	spin_lock(&session->back_lock);
 	task = (struct iscsi_task *)sc->SCp.ptr;
 	if (!task) {
 		/*
@@ -1998,8 +2001,11 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 		 * so let timeout code complete it now.
 		 */
 		rc = BLK_EH_DONE;
+		spin_unlock(&session->back_lock);
 		goto done;
 	}
+	__iscsi_get_task(task);
+	spin_unlock(&session->back_lock);
 
 	if (session->state != ISCSI_STATE_LOGGED_IN) {
 		/*
@@ -2058,6 +2064,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 		goto done;
 	}
 
+	spin_lock(&session->back_lock);
 	for (i = 0; i < conn->session->cmds_max; i++) {
 		running_task = conn->session->cmds[i];
 		if (!running_task->sc || running_task == task ||
@@ -2090,10 +2097,12 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 				     "last xfer %lu/%lu. Last check %lu.\n",
 				     task->last_xfer, running_task->last_xfer,
 				     task->last_timeout);
+			spin_unlock(&session->back_lock);
 			rc = BLK_EH_RESET_TIMER;
 			goto done;
 		}
 	}
+	spin_unlock(&session->back_lock);
 
 	/* Assumes nop timeout is shorter than scsi cmd timeout */
 	if (task->have_checked_conn)
@@ -2115,9 +2124,12 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 	rc = BLK_EH_RESET_TIMER;
 
 done:
-	if (task)
-		task->last_timeout = jiffies;
 	spin_unlock_bh(&session->frwd_lock);
+
+	if (task) {
+		task->last_timeout = jiffies;
+		iscsi_put_task(task);
+	}
 	ISCSI_DBG_EH(session, "return %s\n", rc == BLK_EH_RESET_TIMER ?
 		     "timer reset" : "shutdown or nh");
 	return rc;
@@ -2225,15 +2237,20 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 	conn->eh_abort_cnt++;
 	age = session->age;
 
+	spin_lock(&session->back_lock);
 	task = (struct iscsi_task *)sc->SCp.ptr;
-	ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n",
-		     sc, task->itt);
-
-	/* task completed before time out */
-	if (!task->sc) {
+	if (!task || !task->sc) {
+		/* task completed before time out */
 		ISCSI_DBG_EH(session, "sc completed while abort in progress\n");
-		goto success;
+
+		spin_unlock(&session->back_lock);
+		spin_unlock_bh(&session->frwd_lock);
+		mutex_unlock(&session->eh_mutex);
+		return SUCCESS;
 	}
+	ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
+	__iscsi_get_task(task);
+	spin_unlock(&session->back_lock);
 
 	if (task->state == ISCSI_TASK_PENDING) {
 		fail_scsi_task(task, DID_ABORT);
@@ -2295,6 +2312,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 success_unlocked:
 	ISCSI_DBG_EH(session, "abort success [sc %p itt 0x%x]\n",
 		     sc, task->itt);
+	iscsi_put_task(task);
 	mutex_unlock(&session->eh_mutex);
 	return SUCCESS;
 
@@ -2303,6 +2321,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 failed_unlocked:
 	ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
 		     task ? task->itt : 0);
+	iscsi_put_task(task);
 	mutex_unlock(&session->eh_mutex);
 	return FAILED;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/9] libiscsi: fix iscsi host workq destruction
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (2 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 3/9] libiscsi: fix iscsi_task use after free Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 5/9] libiscsi: add helper to calc max scsi cmds per session Mike Christie
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

We allocate the iscsi host workq in iscsi_host_alloc so iscsi_host_free
should do the destruction. Drivers can then do their error/goto handling
and call iscsi_host_free to clean up what has been allocated in
iscsi_host_alloc.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/libiscsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index ec159bcb7460..b271d3accd2a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2738,8 +2738,6 @@ void iscsi_host_remove(struct Scsi_Host *shost)
 		flush_signals(current);
 
 	scsi_remove_host(shost);
-	if (ihost->workq)
-		destroy_workqueue(ihost->workq);
 }
 EXPORT_SYMBOL_GPL(iscsi_host_remove);
 
@@ -2747,6 +2745,9 @@ void iscsi_host_free(struct Scsi_Host *shost)
 {
 	struct iscsi_host *ihost = shost_priv(shost);
 
+	if (ihost->workq)
+		destroy_workqueue(ihost->workq);
+
 	kfree(ihost->netdev);
 	kfree(ihost->hwaddress);
 	kfree(ihost->initiatorname);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/9] libiscsi: add helper to calc max scsi cmds per session
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (3 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 4/9] libiscsi: fix iscsi host workq destruction Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization Mike Christie
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

This patch just breaks out the code that calculates the number
of scsi cmds that will be used for a scsi session. It also adds
a check that we don't go over the host's can_queue value.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/libiscsi.c | 86 ++++++++++++++++++++++++++---------------
 include/scsi/libiscsi.h |  2 +
 2 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b271d3accd2a..f64e2077754c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2648,6 +2648,56 @@ void iscsi_pool_free(struct iscsi_pool *q)
 }
 EXPORT_SYMBOL_GPL(iscsi_pool_free);
 
+int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
+				 uint16_t requested_cmds_max)
+{
+	int scsi_cmds, total_cmds = requested_cmds_max;
+
+check:
+	if (!total_cmds)
+		total_cmds = ISCSI_DEF_XMIT_CMDS_MAX;
+	/*
+	 * The iscsi layer needs some tasks for nop handling and tmfs,
+	 * so the cmds_max must at least be greater than ISCSI_MGMT_CMDS_MAX
+	 * + 1 command for scsi IO.
+	 */
+	if (total_cmds < ISCSI_TOTAL_CMDS_MIN) {
+		printk(KERN_ERR "iscsi: invalid max cmds of %d. Must be a power of two that is at least %d.\n",
+		       total_cmds, ISCSI_TOTAL_CMDS_MIN);
+		return -EINVAL;
+	}
+
+	if (total_cmds > ISCSI_TOTAL_CMDS_MAX) {
+		printk(KERN_INFO "iscsi: invalid max cmds of %d. Must be a power of 2 less than or equal to %d. Using %d.\n",
+		       requested_cmds_max, ISCSI_TOTAL_CMDS_MAX,
+		       ISCSI_TOTAL_CMDS_MAX);
+		total_cmds = ISCSI_TOTAL_CMDS_MAX;
+	}
+
+	if (!is_power_of_2(total_cmds)) {
+		total_cmds = rounddown_pow_of_two(total_cmds);
+		if (total_cmds < ISCSI_TOTAL_CMDS_MIN) {
+			printk(KERN_ERR "iscsi: invalid max cmds of %d. Must be a power of 2 greater than %d.\n", requested_cmds_max, ISCSI_TOTAL_CMDS_MIN);
+			return -EINVAL;
+		}
+
+		printk(KERN_INFO "iscsi: invalid max cmds %d. Must be a power of 2. Rounding max cmds down to %d.\n",
+		       requested_cmds_max, total_cmds);
+	}
+
+	scsi_cmds = total_cmds - ISCSI_MGMT_CMDS_MAX;
+	if (shost->can_queue && scsi_cmds > shost->can_queue) {
+		total_cmds = shost->can_queue;
+
+		printk(KERN_INFO "iscsi: requested max cmds %u is higher than driver limit. Using driver limit %u\n",
+		       requested_cmds_max, shost->can_queue);
+		goto check;
+	}
+
+	return scsi_cmds;
+}
+EXPORT_SYMBOL_GPL(iscsi_host_get_max_scsi_cmds);
+
 /**
  * iscsi_host_add - add host to system
  * @shost: scsi host
@@ -2801,7 +2851,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
 	struct iscsi_host *ihost = shost_priv(shost);
 	struct iscsi_session *session;
 	struct iscsi_cls_session *cls_session;
-	int cmd_i, scsi_cmds, total_cmds = cmds_max;
+	int cmd_i, scsi_cmds;
 	unsigned long flags;
 
 	spin_lock_irqsave(&ihost->lock, flags);
@@ -2812,37 +2862,9 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
 	ihost->num_sessions++;
 	spin_unlock_irqrestore(&ihost->lock, flags);
 
-	if (!total_cmds)
-		total_cmds = ISCSI_DEF_XMIT_CMDS_MAX;
-	/*
-	 * The iscsi layer needs some tasks for nop handling and tmfs,
-	 * so the cmds_max must at least be greater than ISCSI_MGMT_CMDS_MAX
-	 * + 1 command for scsi IO.
-	 */
-	if (total_cmds < ISCSI_TOTAL_CMDS_MIN) {
-		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
-		       "must be a power of two that is at least %d.\n",
-		       total_cmds, ISCSI_TOTAL_CMDS_MIN);
+	scsi_cmds = iscsi_host_get_max_scsi_cmds(shost, cmds_max);
+	if (scsi_cmds < 0)
 		goto dec_session_count;
-	}
-
-	if (total_cmds > ISCSI_TOTAL_CMDS_MAX) {
-		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
-		       "must be a power of 2 less than or equal to %d.\n",
-		       cmds_max, ISCSI_TOTAL_CMDS_MAX);
-		total_cmds = ISCSI_TOTAL_CMDS_MAX;
-	}
-
-	if (!is_power_of_2(total_cmds)) {
-		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
-		       "must be a power of 2.\n", total_cmds);
-		total_cmds = rounddown_pow_of_two(total_cmds);
-		if (total_cmds < ISCSI_TOTAL_CMDS_MIN)
-			goto dec_session_count;
-		printk(KERN_INFO "iscsi: Rounding can_queue to %d.\n",
-		       total_cmds);
-	}
-	scsi_cmds = total_cmds - ISCSI_MGMT_CMDS_MAX;
 
 	cls_session = iscsi_alloc_session(shost, iscsit,
 					  sizeof(struct iscsi_session) +
@@ -2858,7 +2880,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
 	session->lu_reset_timeout = 15;
 	session->abort_timeout = 10;
 	session->scsi_cmds_max = scsi_cmds;
-	session->cmds_max = total_cmds;
+	session->cmds_max = scsi_cmds + ISCSI_MGMT_CMDS_MAX;
 	session->queued_cmdsn = session->cmdsn = initial_cmdsn;
 	session->exp_cmdsn = initial_cmdsn + 1;
 	session->max_cmdsn = initial_cmdsn + 1;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 44a9554aea62..02f966e9358f 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -395,6 +395,8 @@ extern struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
 extern void iscsi_host_remove(struct Scsi_Host *shost);
 extern void iscsi_host_free(struct Scsi_Host *shost);
 extern int iscsi_target_alloc(struct scsi_target *starget);
+extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
+					uint16_t requested_cmds_max);
 
 /*
  * session management
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (4 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 5/9] libiscsi: add helper to calc max scsi cmds per session Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03 23:33   ` Lee Duncan
  2021-02-03  1:33 ` [PATCH 7/9] libiscsi: reset max/exp cmdsn during recovery Mike Christie
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

We are setting the shost's can_queue after we add the host which is
too late, because scsi-ml will have allocated the tag set based on
the can_queue value at that time. This patch has us use the
iscsi_host_get_max_scsi_cmds helper to figure out the number of
scsi cmds.

It also fixes up the template can_queue so it reflects the max scsi
cmds we can support like how other drivers work.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/iscsi_tcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index a9ce6298b935..dd33ce0e3737 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -847,6 +847,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
 	struct iscsi_session *session;
 	struct iscsi_sw_tcp_host *tcp_sw_host;
 	struct Scsi_Host *shost;
+	int rc;
 
 	if (ep) {
 		printk(KERN_ERR "iscsi_tcp: invalid ep %p.\n", ep);
@@ -864,6 +865,11 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
 	shost->max_channel = 0;
 	shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
 
+	rc = iscsi_host_get_max_scsi_cmds(shost, cmds_max);
+	if (rc < 0)
+		goto free_host;
+	shost->can_queue = rc;
+
 	if (iscsi_host_add(shost, NULL))
 		goto free_host;
 
@@ -878,7 +884,6 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
 	tcp_sw_host = iscsi_host_priv(shost);
 	tcp_sw_host->session = session;
 
-	shost->can_queue = session->scsi_cmds_max;
 	if (iscsi_tcp_r2tpool_alloc(session))
 		goto remove_session;
 	return cls_session;
@@ -981,7 +986,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = {
 	.name			= "iSCSI Initiator over TCP/IP",
 	.queuecommand           = iscsi_queuecommand,
 	.change_queue_depth	= scsi_change_queue_depth,
-	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
+	.can_queue		= ISCSI_TOTAL_CMDS_MAX,
 	.sg_tablesize		= 4096,
 	.max_sectors		= 0xFFFF,
 	.cmd_per_lun		= ISCSI_DEF_CMD_PER_LUN,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 7/9] libiscsi: reset max/exp cmdsn during recovery
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (5 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 8/9] qla4xxx: use iscsi_is_session_online Mike Christie
  2021-02-03  1:33 ` [PATCH 9/9] iscsi class: drop session lock in iscsi_session_chkready Mike Christie
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

If we lose the session then relogin, but the new cmdsn window has
shrunk (due to something like an admin changing a setting) we will
have the old exp/max_cmdsn values and will never be able to update
them. For example, max_cmdsn would be 64, but if on the target the
user set the window to be smaller then the target could try to return
the max_cmdsn as 32. We will see that new max_cmdsn in the rsp but
because it's lower than the old max_cmdsn when the window was larger
we will not update it.

So this patch has us reset the windown values during session
cleanup so they can be updated after a new login.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/libiscsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index f64e2077754c..7ad11e42306d 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3273,6 +3273,13 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
 		session->leadconn = conn;
 	spin_unlock_bh(&session->frwd_lock);
 
+	/*
+	 * The target could have reduced it's window size between logins, so
+	 * we have to reset max/exp cmdsn so we can see the new values.
+	 */
+	spin_lock_bh(&session->back_lock);
+	session->max_cmdsn = session->exp_cmdsn = session->cmdsn + 1;
+	spin_unlock_bh(&session->back_lock);
 	/*
 	 * Unblock xmitworker(), Login Phase will pass through.
 	 */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 8/9] qla4xxx: use iscsi_is_session_online
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (6 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 7/9] libiscsi: reset max/exp cmdsn during recovery Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  2021-02-03  1:33 ` [PATCH 9/9] iscsi class: drop session lock in iscsi_session_chkready Mike Christie
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

__qla4xxx_is_chap_active just wants to know if a session is online and
does not care about why it's not, so this has it use
iscsi_is_session_online.

This is not a bug now, but the next patch changes the behavior of
iscsi_session_chkready so this patch just prepares the driver for that
change.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/qla4xxx/ql4_os.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index a4b014e1cd8c..7bd9a4a04ad5 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -841,7 +841,7 @@ static int __qla4xxx_is_chap_active(struct device *dev, void *data)
 	sess = cls_session->dd_data;
 	ddb_entry = sess->dd_data;
 
-	if (iscsi_session_chkready(cls_session))
+	if (iscsi_is_session_online(cls_session))
 		goto exit_is_chap_active;
 
 	if (ddb_entry->chap_tbl_idx == *chap_tbl_idx)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 9/9] iscsi class: drop session lock in iscsi_session_chkready
  2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
                   ` (7 preceding siblings ...)
  2021-02-03  1:33 ` [PATCH 8/9] qla4xxx: use iscsi_is_session_online Mike Christie
@ 2021-02-03  1:33 ` Mike Christie
  8 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03  1:33 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie

The session lock in iscsi_session_chkready is not needed because when we
transition from logged into to another state we will block and/or
remove the devices under the session, so no new IO will be sent to the
drivers after the block/remove. IO that races with the block/removal is
cleaned up by the drivers when it handles all outstanding IO, so this
just added an extra lock in the main IO path. This patch removes the
lock like other transport classes.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 2e68c0a87698..969d24d580e2 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1701,10 +1701,8 @@ static const char *iscsi_session_state_name(int state)
 
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
-	unsigned long flags;
 	int err;
 
-	spin_lock_irqsave(&session->lock, flags);
 	switch (session->state) {
 	case ISCSI_SESSION_LOGGED_IN:
 		err = 0;
@@ -1719,7 +1717,6 @@ int iscsi_session_chkready(struct iscsi_cls_session *session)
 		err = DID_NO_CONNECT << 16;
 		break;
 	}
-	spin_unlock_irqrestore(&session->lock, flags);
 	return err;
 }
 EXPORT_SYMBOL_GPL(iscsi_session_chkready);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
  2021-02-03  1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie
  2021-02-03 10:19     ` Dan Carpenter
@ 2021-02-03 10:19     ` Dan Carpenter
  0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw)
  To: kbuild, Mike Christie, lduncan, cleech, martin.petersen,
	linux-scsi, james.bottomley
  Cc: lkp, kbuild-all, lutianxiong, linfeilong, liuzhiqiang26,
	haowenchao, Mike Christie

[-- Attachment #1: Type: text/plain, Size: 10189 bytes --]

Hi Mike,

url:    https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-m021-20210202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)

vim +586 drivers/scsi/libiscsi_tcp.c

f7dbf0662a0167 Mike Christie     2021-02-02  529  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
a081c13e39b5c1 Mike Christie     2008-12-02  530  {
a081c13e39b5c1 Mike Christie     2008-12-02  531  	struct iscsi_session *session = conn->session;
f7dbf0662a0167 Mike Christie     2021-02-02  532  	struct iscsi_tcp_task *tcp_task;
f7dbf0662a0167 Mike Christie     2021-02-02  533  	struct iscsi_tcp_conn *tcp_conn;
f7dbf0662a0167 Mike Christie     2021-02-02  534  	struct iscsi_r2t_rsp *rhdr;
a081c13e39b5c1 Mike Christie     2008-12-02  535  	struct iscsi_r2t_info *r2t;
f7dbf0662a0167 Mike Christie     2021-02-02  536  	struct iscsi_task *task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  537  	u32 data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  538  	u32 data_offset;
f7dbf0662a0167 Mike Christie     2021-02-02  539  	int r2tsn;
a081c13e39b5c1 Mike Christie     2008-12-02  540  	int rc;
a081c13e39b5c1 Mike Christie     2008-12-02  541  
f7dbf0662a0167 Mike Christie     2021-02-02  542  	spin_lock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  543  	task = iscsi_itt_to_ctask(conn, hdr->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  544  	if (!task) {
f7dbf0662a0167 Mike Christie     2021-02-02  545  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  546  		return ISCSI_ERR_BAD_ITT;
f7dbf0662a0167 Mike Christie     2021-02-02 @547  	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
                                                                   ^^^^^^^^
New unchecked dereference.

f7dbf0662a0167 Mike Christie     2021-02-02  548  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  549  		return ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  550  	}
f7dbf0662a0167 Mike Christie     2021-02-02  551  	/*
f7dbf0662a0167 Mike Christie     2021-02-02  552  	 * A bad target might complete the cmd before we have handled R2Ts
f7dbf0662a0167 Mike Christie     2021-02-02  553  	 * so get a ref to the task that will be dropped in the xmit path.
f7dbf0662a0167 Mike Christie     2021-02-02  554  	 */
f7dbf0662a0167 Mike Christie     2021-02-02  555  	if (task->state != ISCSI_TASK_RUNNING) {
f7dbf0662a0167 Mike Christie     2021-02-02  556  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  557  		/* Let the path that got the early rsp complete it */
f7dbf0662a0167 Mike Christie     2021-02-02  558  		return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  559  	}
f7dbf0662a0167 Mike Christie     2021-02-02  560  	task->last_xfer = jiffies;
f7dbf0662a0167 Mike Christie     2021-02-02  561  	__iscsi_get_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  562  
f7dbf0662a0167 Mike Christie     2021-02-02  563  	tcp_conn = conn->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  564  	rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
f7dbf0662a0167 Mike Christie     2021-02-02  565  	/* fill-in new R2T associated with the task */
f7dbf0662a0167 Mike Christie     2021-02-02  566  	iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
f7dbf0662a0167 Mike Christie     2021-02-02  567  	spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  568  
a081c13e39b5c1 Mike Christie     2008-12-02  569  	if (tcp_conn->in.datalen) {
a081c13e39b5c1 Mike Christie     2008-12-02  570  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  571  				  "invalid R2t with datalen %d\n",
a081c13e39b5c1 Mike Christie     2008-12-02  572  				  tcp_conn->in.datalen);
f7dbf0662a0167 Mike Christie     2021-02-02  573  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  574  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  575  	}
a081c13e39b5c1 Mike Christie     2008-12-02  576  
f7dbf0662a0167 Mike Christie     2021-02-02  577  	tcp_task = task->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  578  	r2tsn = be32_to_cpu(rhdr->r2tsn);
a081c13e39b5c1 Mike Christie     2008-12-02  579  	if (tcp_task->exp_datasn != r2tsn){
0ab1c2529e6a70 Mike Christie     2009-03-05  580  		ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n",
0ab1c2529e6a70 Mike Christie     2009-03-05  581  			      tcp_task->exp_datasn, r2tsn);
f7dbf0662a0167 Mike Christie     2021-02-02  582  		rc = ISCSI_ERR_R2TSN;
f7dbf0662a0167 Mike Christie     2021-02-02  583  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  584  	}
a081c13e39b5c1 Mike Christie     2008-12-02  585  
a081c13e39b5c1 Mike Christie     2008-12-02 @586  	if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) {
                                                             ^^^^^^^^
Checked too late.

a081c13e39b5c1 Mike Christie     2008-12-02  587  		iscsi_conn_printk(KERN_INFO, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  588  				  "dropping R2T itt %d in recovery.\n",
a081c13e39b5c1 Mike Christie     2008-12-02  589  				  task->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  590  		rc = 0;
f7dbf0662a0167 Mike Christie     2021-02-02  591  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  592  	}
a081c13e39b5c1 Mike Christie     2008-12-02  593  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  594  	data_length = be32_to_cpu(rhdr->data_length);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  595  	if (data_length == 0) {
a081c13e39b5c1 Mike Christie     2008-12-02  596  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  597  				  "invalid R2T with zero data len\n");
f7dbf0662a0167 Mike Christie     2021-02-02  598  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  599  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  600  	}
a081c13e39b5c1 Mike Christie     2008-12-02  601  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  602  	if (data_length > session->max_burst)
0ab1c2529e6a70 Mike Christie     2009-03-05  603  		ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max "
0ab1c2529e6a70 Mike Christie     2009-03-05  604  			      "burst %u. Attempting to execute request.\n",
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  605  			      data_length, session->max_burst);
a081c13e39b5c1 Mike Christie     2008-12-02  606  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  607  	data_offset = be32_to_cpu(rhdr->data_offset);
ae3d56d81507c3 Christoph Hellwig 2019-01-29  608  	if (data_offset + data_length > task->sc->sdb.length) {
a081c13e39b5c1 Mike Christie     2008-12-02  609  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  610  				  "invalid R2T with data len %u at offset %u "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  611  				  "and total length %d\n", data_length,
ae3d56d81507c3 Christoph Hellwig 2019-01-29  612  				  data_offset, task->sc->sdb.length);
f7dbf0662a0167 Mike Christie     2021-02-02  613  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  614  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  615  	}
a081c13e39b5c1 Mike Christie     2008-12-02  616  
659743b02c4110 Shlomo Pongratz   2014-02-07  617  	spin_lock(&tcp_task->pool2queue);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  618  	rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *));
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  619  	if (!rc) {
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  620  		iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  621  				  "Target has sent more R2Ts than it "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  622  				  "negotiated for or driver has leaked.\n");
659743b02c4110 Shlomo Pongratz   2014-02-07  623  		spin_unlock(&tcp_task->pool2queue);
f7dbf0662a0167 Mike Christie     2021-02-02  624  		rc = ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  625  		goto put_task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  626  	}
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  627  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  628  	r2t->exp_statsn = rhdr->statsn;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  629  	r2t->data_length = data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  630  	r2t->data_offset = data_offset;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  631  
a081c13e39b5c1 Mike Christie     2008-12-02  632  	r2t->ttt = rhdr->ttt; /* no flip */
a081c13e39b5c1 Mike Christie     2008-12-02  633  	r2t->datasn = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  634  	r2t->sent = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  635  
a081c13e39b5c1 Mike Christie     2008-12-02  636  	tcp_task->exp_datasn = r2tsn + 1;
7acd72eb85f1c7 Stefani Seibold   2009-12-21  637  	kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
a081c13e39b5c1 Mike Christie     2008-12-02  638  	conn->r2t_pdus_cnt++;
659743b02c4110 Shlomo Pongratz   2014-02-07  639  	spin_unlock(&tcp_task->pool2queue);
a081c13e39b5c1 Mike Christie     2008-12-02  640  
a081c13e39b5c1 Mike Christie     2008-12-02  641  	iscsi_requeue_task(task);
a081c13e39b5c1 Mike Christie     2008-12-02  642  	return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  643  
f7dbf0662a0167 Mike Christie     2021-02-02  644  put_task:
f7dbf0662a0167 Mike Christie     2021-02-02  645  	iscsi_put_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  646  	return rc;
a081c13e39b5c1 Mike Christie     2008-12-02  647  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38567 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
@ 2021-02-03 10:19     ` Dan Carpenter
  0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 10336 bytes --]

Hi Mike,

url:    https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-m021-20210202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)

vim +586 drivers/scsi/libiscsi_tcp.c

f7dbf0662a0167 Mike Christie     2021-02-02  529  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
a081c13e39b5c1 Mike Christie     2008-12-02  530  {
a081c13e39b5c1 Mike Christie     2008-12-02  531  	struct iscsi_session *session = conn->session;
f7dbf0662a0167 Mike Christie     2021-02-02  532  	struct iscsi_tcp_task *tcp_task;
f7dbf0662a0167 Mike Christie     2021-02-02  533  	struct iscsi_tcp_conn *tcp_conn;
f7dbf0662a0167 Mike Christie     2021-02-02  534  	struct iscsi_r2t_rsp *rhdr;
a081c13e39b5c1 Mike Christie     2008-12-02  535  	struct iscsi_r2t_info *r2t;
f7dbf0662a0167 Mike Christie     2021-02-02  536  	struct iscsi_task *task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  537  	u32 data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  538  	u32 data_offset;
f7dbf0662a0167 Mike Christie     2021-02-02  539  	int r2tsn;
a081c13e39b5c1 Mike Christie     2008-12-02  540  	int rc;
a081c13e39b5c1 Mike Christie     2008-12-02  541  
f7dbf0662a0167 Mike Christie     2021-02-02  542  	spin_lock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  543  	task = iscsi_itt_to_ctask(conn, hdr->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  544  	if (!task) {
f7dbf0662a0167 Mike Christie     2021-02-02  545  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  546  		return ISCSI_ERR_BAD_ITT;
f7dbf0662a0167 Mike Christie     2021-02-02 @547  	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
                                                                   ^^^^^^^^
New unchecked dereference.

f7dbf0662a0167 Mike Christie     2021-02-02  548  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  549  		return ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  550  	}
f7dbf0662a0167 Mike Christie     2021-02-02  551  	/*
f7dbf0662a0167 Mike Christie     2021-02-02  552  	 * A bad target might complete the cmd before we have handled R2Ts
f7dbf0662a0167 Mike Christie     2021-02-02  553  	 * so get a ref to the task that will be dropped in the xmit path.
f7dbf0662a0167 Mike Christie     2021-02-02  554  	 */
f7dbf0662a0167 Mike Christie     2021-02-02  555  	if (task->state != ISCSI_TASK_RUNNING) {
f7dbf0662a0167 Mike Christie     2021-02-02  556  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  557  		/* Let the path that got the early rsp complete it */
f7dbf0662a0167 Mike Christie     2021-02-02  558  		return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  559  	}
f7dbf0662a0167 Mike Christie     2021-02-02  560  	task->last_xfer = jiffies;
f7dbf0662a0167 Mike Christie     2021-02-02  561  	__iscsi_get_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  562  
f7dbf0662a0167 Mike Christie     2021-02-02  563  	tcp_conn = conn->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  564  	rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
f7dbf0662a0167 Mike Christie     2021-02-02  565  	/* fill-in new R2T associated with the task */
f7dbf0662a0167 Mike Christie     2021-02-02  566  	iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
f7dbf0662a0167 Mike Christie     2021-02-02  567  	spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  568  
a081c13e39b5c1 Mike Christie     2008-12-02  569  	if (tcp_conn->in.datalen) {
a081c13e39b5c1 Mike Christie     2008-12-02  570  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  571  				  "invalid R2t with datalen %d\n",
a081c13e39b5c1 Mike Christie     2008-12-02  572  				  tcp_conn->in.datalen);
f7dbf0662a0167 Mike Christie     2021-02-02  573  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  574  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  575  	}
a081c13e39b5c1 Mike Christie     2008-12-02  576  
f7dbf0662a0167 Mike Christie     2021-02-02  577  	tcp_task = task->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  578  	r2tsn = be32_to_cpu(rhdr->r2tsn);
a081c13e39b5c1 Mike Christie     2008-12-02  579  	if (tcp_task->exp_datasn != r2tsn){
0ab1c2529e6a70 Mike Christie     2009-03-05  580  		ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n",
0ab1c2529e6a70 Mike Christie     2009-03-05  581  			      tcp_task->exp_datasn, r2tsn);
f7dbf0662a0167 Mike Christie     2021-02-02  582  		rc = ISCSI_ERR_R2TSN;
f7dbf0662a0167 Mike Christie     2021-02-02  583  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  584  	}
a081c13e39b5c1 Mike Christie     2008-12-02  585  
a081c13e39b5c1 Mike Christie     2008-12-02 @586  	if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) {
                                                             ^^^^^^^^
Checked too late.

a081c13e39b5c1 Mike Christie     2008-12-02  587  		iscsi_conn_printk(KERN_INFO, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  588  				  "dropping R2T itt %d in recovery.\n",
a081c13e39b5c1 Mike Christie     2008-12-02  589  				  task->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  590  		rc = 0;
f7dbf0662a0167 Mike Christie     2021-02-02  591  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  592  	}
a081c13e39b5c1 Mike Christie     2008-12-02  593  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  594  	data_length = be32_to_cpu(rhdr->data_length);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  595  	if (data_length == 0) {
a081c13e39b5c1 Mike Christie     2008-12-02  596  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  597  				  "invalid R2T with zero data len\n");
f7dbf0662a0167 Mike Christie     2021-02-02  598  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  599  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  600  	}
a081c13e39b5c1 Mike Christie     2008-12-02  601  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  602  	if (data_length > session->max_burst)
0ab1c2529e6a70 Mike Christie     2009-03-05  603  		ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max "
0ab1c2529e6a70 Mike Christie     2009-03-05  604  			      "burst %u. Attempting to execute request.\n",
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  605  			      data_length, session->max_burst);
a081c13e39b5c1 Mike Christie     2008-12-02  606  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  607  	data_offset = be32_to_cpu(rhdr->data_offset);
ae3d56d81507c3 Christoph Hellwig 2019-01-29  608  	if (data_offset + data_length > task->sc->sdb.length) {
a081c13e39b5c1 Mike Christie     2008-12-02  609  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  610  				  "invalid R2T with data len %u at offset %u "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  611  				  "and total length %d\n", data_length,
ae3d56d81507c3 Christoph Hellwig 2019-01-29  612  				  data_offset, task->sc->sdb.length);
f7dbf0662a0167 Mike Christie     2021-02-02  613  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  614  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  615  	}
a081c13e39b5c1 Mike Christie     2008-12-02  616  
659743b02c4110 Shlomo Pongratz   2014-02-07  617  	spin_lock(&tcp_task->pool2queue);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  618  	rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *));
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  619  	if (!rc) {
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  620  		iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  621  				  "Target has sent more R2Ts than it "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  622  				  "negotiated for or driver has leaked.\n");
659743b02c4110 Shlomo Pongratz   2014-02-07  623  		spin_unlock(&tcp_task->pool2queue);
f7dbf0662a0167 Mike Christie     2021-02-02  624  		rc = ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  625  		goto put_task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  626  	}
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  627  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  628  	r2t->exp_statsn = rhdr->statsn;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  629  	r2t->data_length = data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  630  	r2t->data_offset = data_offset;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  631  
a081c13e39b5c1 Mike Christie     2008-12-02  632  	r2t->ttt = rhdr->ttt; /* no flip */
a081c13e39b5c1 Mike Christie     2008-12-02  633  	r2t->datasn = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  634  	r2t->sent = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  635  
a081c13e39b5c1 Mike Christie     2008-12-02  636  	tcp_task->exp_datasn = r2tsn + 1;
7acd72eb85f1c7 Stefani Seibold   2009-12-21  637  	kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
a081c13e39b5c1 Mike Christie     2008-12-02  638  	conn->r2t_pdus_cnt++;
659743b02c4110 Shlomo Pongratz   2014-02-07  639  	spin_unlock(&tcp_task->pool2queue);
a081c13e39b5c1 Mike Christie     2008-12-02  640  
a081c13e39b5c1 Mike Christie     2008-12-02  641  	iscsi_requeue_task(task);
a081c13e39b5c1 Mike Christie     2008-12-02  642  	return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  643  
f7dbf0662a0167 Mike Christie     2021-02-02  644  put_task:
f7dbf0662a0167 Mike Christie     2021-02-02  645  	iscsi_put_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  646  	return rc;
a081c13e39b5c1 Mike Christie     2008-12-02  647  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38567 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
@ 2021-02-03 10:19     ` Dan Carpenter
  0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 10336 bytes --]

Hi Mike,

url:    https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-m021-20210202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)

vim +586 drivers/scsi/libiscsi_tcp.c

f7dbf0662a0167 Mike Christie     2021-02-02  529  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
a081c13e39b5c1 Mike Christie     2008-12-02  530  {
a081c13e39b5c1 Mike Christie     2008-12-02  531  	struct iscsi_session *session = conn->session;
f7dbf0662a0167 Mike Christie     2021-02-02  532  	struct iscsi_tcp_task *tcp_task;
f7dbf0662a0167 Mike Christie     2021-02-02  533  	struct iscsi_tcp_conn *tcp_conn;
f7dbf0662a0167 Mike Christie     2021-02-02  534  	struct iscsi_r2t_rsp *rhdr;
a081c13e39b5c1 Mike Christie     2008-12-02  535  	struct iscsi_r2t_info *r2t;
f7dbf0662a0167 Mike Christie     2021-02-02  536  	struct iscsi_task *task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  537  	u32 data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  538  	u32 data_offset;
f7dbf0662a0167 Mike Christie     2021-02-02  539  	int r2tsn;
a081c13e39b5c1 Mike Christie     2008-12-02  540  	int rc;
a081c13e39b5c1 Mike Christie     2008-12-02  541  
f7dbf0662a0167 Mike Christie     2021-02-02  542  	spin_lock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  543  	task = iscsi_itt_to_ctask(conn, hdr->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  544  	if (!task) {
f7dbf0662a0167 Mike Christie     2021-02-02  545  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  546  		return ISCSI_ERR_BAD_ITT;
f7dbf0662a0167 Mike Christie     2021-02-02 @547  	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
                                                                   ^^^^^^^^
New unchecked dereference.

f7dbf0662a0167 Mike Christie     2021-02-02  548  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  549  		return ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  550  	}
f7dbf0662a0167 Mike Christie     2021-02-02  551  	/*
f7dbf0662a0167 Mike Christie     2021-02-02  552  	 * A bad target might complete the cmd before we have handled R2Ts
f7dbf0662a0167 Mike Christie     2021-02-02  553  	 * so get a ref to the task that will be dropped in the xmit path.
f7dbf0662a0167 Mike Christie     2021-02-02  554  	 */
f7dbf0662a0167 Mike Christie     2021-02-02  555  	if (task->state != ISCSI_TASK_RUNNING) {
f7dbf0662a0167 Mike Christie     2021-02-02  556  		spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  557  		/* Let the path that got the early rsp complete it */
f7dbf0662a0167 Mike Christie     2021-02-02  558  		return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  559  	}
f7dbf0662a0167 Mike Christie     2021-02-02  560  	task->last_xfer = jiffies;
f7dbf0662a0167 Mike Christie     2021-02-02  561  	__iscsi_get_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  562  
f7dbf0662a0167 Mike Christie     2021-02-02  563  	tcp_conn = conn->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  564  	rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
f7dbf0662a0167 Mike Christie     2021-02-02  565  	/* fill-in new R2T associated with the task */
f7dbf0662a0167 Mike Christie     2021-02-02  566  	iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
f7dbf0662a0167 Mike Christie     2021-02-02  567  	spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie     2021-02-02  568  
a081c13e39b5c1 Mike Christie     2008-12-02  569  	if (tcp_conn->in.datalen) {
a081c13e39b5c1 Mike Christie     2008-12-02  570  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  571  				  "invalid R2t with datalen %d\n",
a081c13e39b5c1 Mike Christie     2008-12-02  572  				  tcp_conn->in.datalen);
f7dbf0662a0167 Mike Christie     2021-02-02  573  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  574  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  575  	}
a081c13e39b5c1 Mike Christie     2008-12-02  576  
f7dbf0662a0167 Mike Christie     2021-02-02  577  	tcp_task = task->dd_data;
f7dbf0662a0167 Mike Christie     2021-02-02  578  	r2tsn = be32_to_cpu(rhdr->r2tsn);
a081c13e39b5c1 Mike Christie     2008-12-02  579  	if (tcp_task->exp_datasn != r2tsn){
0ab1c2529e6a70 Mike Christie     2009-03-05  580  		ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n",
0ab1c2529e6a70 Mike Christie     2009-03-05  581  			      tcp_task->exp_datasn, r2tsn);
f7dbf0662a0167 Mike Christie     2021-02-02  582  		rc = ISCSI_ERR_R2TSN;
f7dbf0662a0167 Mike Christie     2021-02-02  583  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  584  	}
a081c13e39b5c1 Mike Christie     2008-12-02  585  
a081c13e39b5c1 Mike Christie     2008-12-02 @586  	if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) {
                                                             ^^^^^^^^
Checked too late.

a081c13e39b5c1 Mike Christie     2008-12-02  587  		iscsi_conn_printk(KERN_INFO, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  588  				  "dropping R2T itt %d in recovery.\n",
a081c13e39b5c1 Mike Christie     2008-12-02  589  				  task->itt);
f7dbf0662a0167 Mike Christie     2021-02-02  590  		rc = 0;
f7dbf0662a0167 Mike Christie     2021-02-02  591  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  592  	}
a081c13e39b5c1 Mike Christie     2008-12-02  593  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  594  	data_length = be32_to_cpu(rhdr->data_length);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  595  	if (data_length == 0) {
a081c13e39b5c1 Mike Christie     2008-12-02  596  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  597  				  "invalid R2T with zero data len\n");
f7dbf0662a0167 Mike Christie     2021-02-02  598  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  599  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  600  	}
a081c13e39b5c1 Mike Christie     2008-12-02  601  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  602  	if (data_length > session->max_burst)
0ab1c2529e6a70 Mike Christie     2009-03-05  603  		ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max "
0ab1c2529e6a70 Mike Christie     2009-03-05  604  			      "burst %u. Attempting to execute request.\n",
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  605  			      data_length, session->max_burst);
a081c13e39b5c1 Mike Christie     2008-12-02  606  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  607  	data_offset = be32_to_cpu(rhdr->data_offset);
ae3d56d81507c3 Christoph Hellwig 2019-01-29  608  	if (data_offset + data_length > task->sc->sdb.length) {
a081c13e39b5c1 Mike Christie     2008-12-02  609  		iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie     2008-12-02  610  				  "invalid R2T with data len %u at offset %u "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  611  				  "and total length %d\n", data_length,
ae3d56d81507c3 Christoph Hellwig 2019-01-29  612  				  data_offset, task->sc->sdb.length);
f7dbf0662a0167 Mike Christie     2021-02-02  613  		rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie     2021-02-02  614  		goto put_task;
a081c13e39b5c1 Mike Christie     2008-12-02  615  	}
a081c13e39b5c1 Mike Christie     2008-12-02  616  
659743b02c4110 Shlomo Pongratz   2014-02-07  617  	spin_lock(&tcp_task->pool2queue);
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  618  	rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *));
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  619  	if (!rc) {
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  620  		iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  621  				  "Target has sent more R2Ts than it "
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  622  				  "negotiated for or driver has leaked.\n");
659743b02c4110 Shlomo Pongratz   2014-02-07  623  		spin_unlock(&tcp_task->pool2queue);
f7dbf0662a0167 Mike Christie     2021-02-02  624  		rc = ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie     2021-02-02  625  		goto put_task;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  626  	}
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  627  
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  628  	r2t->exp_statsn = rhdr->statsn;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  629  	r2t->data_length = data_length;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  630  	r2t->data_offset = data_offset;
5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  631  
a081c13e39b5c1 Mike Christie     2008-12-02  632  	r2t->ttt = rhdr->ttt; /* no flip */
a081c13e39b5c1 Mike Christie     2008-12-02  633  	r2t->datasn = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  634  	r2t->sent = 0;
a081c13e39b5c1 Mike Christie     2008-12-02  635  
a081c13e39b5c1 Mike Christie     2008-12-02  636  	tcp_task->exp_datasn = r2tsn + 1;
7acd72eb85f1c7 Stefani Seibold   2009-12-21  637  	kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
a081c13e39b5c1 Mike Christie     2008-12-02  638  	conn->r2t_pdus_cnt++;
659743b02c4110 Shlomo Pongratz   2014-02-07  639  	spin_unlock(&tcp_task->pool2queue);
a081c13e39b5c1 Mike Christie     2008-12-02  640  
a081c13e39b5c1 Mike Christie     2008-12-02  641  	iscsi_requeue_task(task);
a081c13e39b5c1 Mike Christie     2008-12-02  642  	return 0;
f7dbf0662a0167 Mike Christie     2021-02-02  643  
f7dbf0662a0167 Mike Christie     2021-02-02  644  put_task:
f7dbf0662a0167 Mike Christie     2021-02-02  645  	iscsi_put_task(task);
f7dbf0662a0167 Mike Christie     2021-02-02  646  	return rc;
a081c13e39b5c1 Mike Christie     2008-12-02  647  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38567 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
  2021-02-03 10:19     ` Dan Carpenter
@ 2021-02-03 17:10       ` Mike Christie
  -1 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03 17:10 UTC (permalink / raw)
  To: Dan Carpenter, kbuild, lduncan, cleech, martin.petersen,
	linux-scsi, james.bottomley
  Cc: lkp, kbuild-all, lutianxiong, linfeilong, liuzhiqiang26, haowenchao

On 2/3/21 4:19 AM, Dan Carpenter wrote:
> Hi Mike,
> 
> url:    https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
> config: i386-randconfig-m021-20210202 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)
> 
> vim +586 drivers/scsi/libiscsi_tcp.c
> 
> f7dbf0662a0167 Mike Christie     2021-02-02  529  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
> a081c13e39b5c1 Mike Christie     2008-12-02  530  {
> a081c13e39b5c1 Mike Christie     2008-12-02  531  	struct iscsi_session *session = conn->session;
> f7dbf0662a0167 Mike Christie     2021-02-02  532  	struct iscsi_tcp_task *tcp_task;
> f7dbf0662a0167 Mike Christie     2021-02-02  533  	struct iscsi_tcp_conn *tcp_conn;
> f7dbf0662a0167 Mike Christie     2021-02-02  534  	struct iscsi_r2t_rsp *rhdr;
> a081c13e39b5c1 Mike Christie     2008-12-02  535  	struct iscsi_r2t_info *r2t;
> f7dbf0662a0167 Mike Christie     2021-02-02  536  	struct iscsi_task *task;
> 5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  537  	u32 data_length;
> 5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  538  	u32 data_offset;
> f7dbf0662a0167 Mike Christie     2021-02-02  539  	int r2tsn;
> a081c13e39b5c1 Mike Christie     2008-12-02  540  	int rc;
> a081c13e39b5c1 Mike Christie     2008-12-02  541  
> f7dbf0662a0167 Mike Christie     2021-02-02  542  	spin_lock(&session->back_lock);
> f7dbf0662a0167 Mike Christie     2021-02-02  543  	task = iscsi_itt_to_ctask(conn, hdr->itt);
> f7dbf0662a0167 Mike Christie     2021-02-02  544  	if (!task) {
> f7dbf0662a0167 Mike Christie     2021-02-02  545  		spin_unlock(&session->back_lock);
> f7dbf0662a0167 Mike Christie     2021-02-02  546  		return ISCSI_ERR_BAD_ITT;
> f7dbf0662a0167 Mike Christie     2021-02-02 @547  	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
>                                                                    ^^^^^^^^
> New unchecked dereference.

I see the issue. iscsi_itt_ctask checks task->sc and if NULL returns NULL.
However, below in this function there is now a not needed task->sc check.
The checker saw that and thinks the above line could be a invalid access.

I'll fix the patch by removing the old check since it's confusing code
that's also not needed since it's done for us now.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
@ 2021-02-03 17:10       ` Mike Christie
  0 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-03 17:10 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]

On 2/3/21 4:19 AM, Dan Carpenter wrote:
> Hi Mike,
> 
> url:    https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
> config: i386-randconfig-m021-20210202 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)
> 
> vim +586 drivers/scsi/libiscsi_tcp.c
> 
> f7dbf0662a0167 Mike Christie     2021-02-02  529  static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
> a081c13e39b5c1 Mike Christie     2008-12-02  530  {
> a081c13e39b5c1 Mike Christie     2008-12-02  531  	struct iscsi_session *session = conn->session;
> f7dbf0662a0167 Mike Christie     2021-02-02  532  	struct iscsi_tcp_task *tcp_task;
> f7dbf0662a0167 Mike Christie     2021-02-02  533  	struct iscsi_tcp_conn *tcp_conn;
> f7dbf0662a0167 Mike Christie     2021-02-02  534  	struct iscsi_r2t_rsp *rhdr;
> a081c13e39b5c1 Mike Christie     2008-12-02  535  	struct iscsi_r2t_info *r2t;
> f7dbf0662a0167 Mike Christie     2021-02-02  536  	struct iscsi_task *task;
> 5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  537  	u32 data_length;
> 5d0fddd0a72d30 Shlomo Pongratz   2014-02-07  538  	u32 data_offset;
> f7dbf0662a0167 Mike Christie     2021-02-02  539  	int r2tsn;
> a081c13e39b5c1 Mike Christie     2008-12-02  540  	int rc;
> a081c13e39b5c1 Mike Christie     2008-12-02  541  
> f7dbf0662a0167 Mike Christie     2021-02-02  542  	spin_lock(&session->back_lock);
> f7dbf0662a0167 Mike Christie     2021-02-02  543  	task = iscsi_itt_to_ctask(conn, hdr->itt);
> f7dbf0662a0167 Mike Christie     2021-02-02  544  	if (!task) {
> f7dbf0662a0167 Mike Christie     2021-02-02  545  		spin_unlock(&session->back_lock);
> f7dbf0662a0167 Mike Christie     2021-02-02  546  		return ISCSI_ERR_BAD_ITT;
> f7dbf0662a0167 Mike Christie     2021-02-02 @547  	} else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
>                                                                    ^^^^^^^^
> New unchecked dereference.

I see the issue. iscsi_itt_ctask checks task->sc and if NULL returns NULL.
However, below in this function there is now a not needed task->sc check.
The checker saw that and thinks the above line could be a invalid access.

I'll fix the patch by removing the old check since it's confusing code
that's also not needed since it's done for us now.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization
  2021-02-03  1:33 ` [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization Mike Christie
@ 2021-02-03 23:33   ` Lee Duncan
  0 siblings, 0 replies; 17+ messages in thread
From: Lee Duncan @ 2021-02-03 23:33 UTC (permalink / raw)
  To: Mike Christie, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao

On 2/2/21 5:33 PM, Mike Christie wrote:
> We are setting the shost's can_queue after we add the host which is
> too late, because scsi-ml will have allocated the tag set based on
> the can_queue value at that time. This patch has us use the
> iscsi_host_get_max_scsi_cmds helper to figure out the number of
> scsi cmds.
> 
> It also fixes up the template can_queue so it reflects the max scsi
> cmds we can support like how other drivers work.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index a9ce6298b935..dd33ce0e3737 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -847,6 +847,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
>  	struct iscsi_session *session;
>  	struct iscsi_sw_tcp_host *tcp_sw_host;
>  	struct Scsi_Host *shost;
> +	int rc;
>  
>  	if (ep) {
>  		printk(KERN_ERR "iscsi_tcp: invalid ep %p.\n", ep);
> @@ -864,6 +865,11 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
>  	shost->max_channel = 0;
>  	shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
>  
> +	rc = iscsi_host_get_max_scsi_cmds(shost, cmds_max);
> +	if (rc < 0)
> +		goto free_host;
> +	shost->can_queue = rc;
> +
>  	if (iscsi_host_add(shost, NULL))
>  		goto free_host;
>  
> @@ -878,7 +884,6 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
>  	tcp_sw_host = iscsi_host_priv(shost);
>  	tcp_sw_host->session = session;
>  
> -	shost->can_queue = session->scsi_cmds_max;
>  	if (iscsi_tcp_r2tpool_alloc(session))
>  		goto remove_session;
>  	return cls_session;
> @@ -981,7 +986,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = {
>  	.name			= "iSCSI Initiator over TCP/IP",
>  	.queuecommand           = iscsi_queuecommand,
>  	.change_queue_depth	= scsi_change_queue_depth,
> -	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
> +	.can_queue		= ISCSI_TOTAL_CMDS_MAX,
>  	.sg_tablesize		= 4096,
>  	.max_sectors		= 0xFFFF,
>  	.cmd_per_lun		= ISCSI_DEF_CMD_PER_LUN,
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/9] libiscsi: fix iscsi_task use after free
  2021-02-07  4:45 [PATCH 0/9 V6] iscsi fixes and cleanups Mike Christie
@ 2021-02-07  4:46 ` Mike Christie
  0 siblings, 0 replies; 17+ messages in thread
From: Mike Christie @ 2021-02-07  4:46 UTC (permalink / raw)
  To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
  Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie, Wu Bo

The following bug was reported and debugged by wubo40@huawei.com:

When testing kernel 4.18 version, NULL pointer dereference problem
occurs
in iscsi_eh_cmd_timed_out function.

I think this bug in the upstream is still exists.

The analysis reasons are as follows:
1)  For some reason, I/O command did not complete within
    the timeout period. The block layer timer works,
    call scsi_times_out() to handle I/O timeout logic.
    At the same time the command just completes.

2)  scsi_times_out() call iscsi_eh_cmd_timed_out()
    to processing timeout logic.  although there is an NULL judgment
        for the task, the task has not been released yet now.

3)  iscsi_complete_task() call __iscsi_put_task(),
    The task reference count reaches zero, the conditions for free task
    is met, then iscsi_free_task () free the task,
    and let sc->SCp.ptr = NULL. After iscsi_eh_cmd_timed_out passes
    the task judgment check, there may be NULL dereference scenarios
    later.

   CPU0                                                CPU3

    |- scsi_times_out()                                 |-
iscsi_complete_task()
    |                                                   |
    |- iscsi_eh_cmd_timed_out()                         |-
__iscsi_put_task()
    |                                                   |
    |- task=sc->SCp.ptr, task is not NUL, check passed  |-
iscsi_free_task(task)
    |                                                   |
    |                                                   |-> sc->SCp.ptr
= NULL
    |                                                   |
    |- task is NULL now, NULL pointer dereference       |
    |                                                   |
   \|/                                                 \|/

Calltrace:
[380751.840862] BUG: unable to handle kernel NULL pointer dereference at
0000000000000138
[380751.843709] PGD 0 P4D 0
[380751.844770] Oops: 0000 [#1] SMP PTI
[380751.846283] CPU: 0 PID: 403 Comm: kworker/0:1H Kdump: loaded
Tainted: G
[380751.851467] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
[380751.856521] Workqueue: kblockd blk_mq_timeout_work
[380751.858527] RIP: 0010:iscsi_eh_cmd_timed_out+0x15e/0x2e0 [libiscsi]
[380751.861129] Code: 83 ea 01 48 8d 74 d0 08 48 8b 10 48 8b 4a 50 48 85
c9 74 2c 48 39 d5 74
[380751.868811] RSP: 0018:ffffc1e280a5fd58 EFLAGS: 00010246
[380751.870978] RAX: ffff9fd1e84e15e0 RBX: ffff9fd1e84e6dd0 RCX:
0000000116acc580
[380751.873791] RDX: ffff9fd1f97a9400 RSI: ffff9fd1e84e1800 RDI:
ffff9fd1e4d6d420
[380751.876059] RBP: ffff9fd1e4d49000 R08: 0000000116acc580 R09:
0000000116acc580
[380751.878284] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff9fd1e6e931e8
[380751.880500] R13: ffff9fd1e84e6ee0 R14: 0000000000000010 R15:
0000000000000003
[380751.882687] FS:  0000000000000000(0000) GS:ffff9fd1fac00000(0000)
knlGS:0000000000000000
[380751.885236] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[380751.887059] CR2: 0000000000000138 CR3: 000000011860a001 CR4:
00000000003606f0
[380751.889308] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[380751.891523] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[380751.893738] Call Trace:
[380751.894639]  scsi_times_out+0x60/0x1c0
[380751.895861]  blk_mq_check_expired+0x144/0x200
[380751.897302]  ? __switch_to_asm+0x35/0x70
[380751.898551]  blk_mq_queue_tag_busy_iter+0x195/0x2e0
[380751.900091]  ? __blk_mq_requeue_request+0x100/0x100
[380751.901611]  ? __switch_to_asm+0x41/0x70
[380751.902853]  ? __blk_mq_requeue_request+0x100/0x100
[380751.904398]  blk_mq_timeout_work+0x54/0x130
[380751.905740]  process_one_work+0x195/0x390
[380751.907228]  worker_thread+0x30/0x390
[380751.908713]  ? process_one_work+0x390/0x390
[380751.910350]  kthread+0x10d/0x130
[380751.911470]  ? kthread_flush_work_fn+0x10/0x10
[380751.913007]  ret_from_fork+0x35/0x40

crash> dis -l iscsi_eh_cmd_timed_out+0x15e
xxxxx/drivers/scsi/libiscsi.c: 2062

1970 enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd
*sc)
{
...
1984         spin_lock_bh(&session->frwd_lock);
1985         task = (struct iscsi_task *)sc->SCp.ptr;
1986         if (!task) {
1987                 /*
1988                  * Raced with completion. Blk layer has taken
ownership
1989                  * so let timeout code complete it now.
1990                  */
1991                 rc = BLK_EH_DONE;
1992                 goto done;
1993         }

...

2052         for (i = 0; i < conn->session->cmds_max; i++) {
2053                 running_task = conn->session->cmds[i];
2054                 if (!running_task->sc || running_task == task ||
2055                      running_task->state != ISCSI_TASK_RUNNING)
2056                         continue;
2057
2058                 /*
2059                  * Only check if cmds started before this one have
made
2060                  * progress, or this could never fail
2061                  */
2062                 if (time_after(running_task->sc->jiffies_at_alloc,
2063                                task->sc->jiffies_at_alloc))    <---
2064                         continue;
2065
...
}

carsh> struct scsi_cmnd ffff9fd1e6e931e8
struct scsi_cmnd {
  ...
  SCp = {
    ptr = 0x0,   <--- iscsi_task
    this_residual = 0,
    ...
  },
}

To prevent this, we take a ref to the cmd under the back (completion)
lock so if the completion side were to call iscsi_complete_task on the
task while the timer/eh paths are not holding the back_lock it will
not be freed from under us.

Note that this requires the previous patch, "libiscsi: drop
taskqueuelock" because bnx2i sleeps in its cleanup_task callout if the
cmd is aborted. If the EH/timer and completion path are racing we don't
know which path will do the last put. The previous patch moved the
operations we needed to do under the forward lock to cleanup_queued_task.
Once that has run we can drop the forward lock for the cmd and bnx2i no
longer has to worry about if the EH, timer or completion path did the
ast put and if the forward lock is held or not since it won't be.

Reported-by: Wu Bo <wubo40@huawei.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/bnx2i/bnx2i_iscsi.c |  2 -
 drivers/scsi/libiscsi.c          | 71 ++++++++++++++++++++------------
 2 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index fdd446765311..1e6d8f62ea3c 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1171,10 +1171,8 @@ static void bnx2i_cleanup_task(struct iscsi_task *task)
 		bnx2i_send_cmd_cleanup_req(hba, task->dd_data);
 
 		spin_unlock_bh(&conn->session->back_lock);
-		spin_unlock_bh(&conn->session->frwd_lock);
 		wait_for_completion_timeout(&bnx2i_conn->cmd_cleanup_cmpl,
 				msecs_to_jiffies(ISCSI_CMD_CLEANUP_TIMEOUT));
-		spin_lock_bh(&conn->session->frwd_lock);
 		spin_lock_bh(&conn->session->back_lock);
 	}
 	bnx2i_iscsi_unmap_sg_list(task->dd_data);
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3d74fdd9f31a..ec159bcb7460 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -587,9 +587,8 @@ static bool cleanup_queued_task(struct iscsi_task *task)
 }
 
 /*
- * session frwd_lock must be held and if not called for a task that is
- * still pending or from the xmit thread, then xmit thread must
- * be suspended.
+ * session frwd lock must be held and if not called for a task that is still
+ * pending or from the xmit thread, then xmit thread must be suspended
  */
 static void fail_scsi_task(struct iscsi_task *task, int err)
 {
@@ -597,16 +596,6 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 	struct scsi_cmnd *sc;
 	int state;
 
-	/*
-	 * if a command completes and we get a successful tmf response
-	 * we will hit this because the scsi eh abort code does not take
-	 * a ref to the task.
-	 */
-	sc = task->sc;
-	if (!sc)
-		return;
-
-	/* regular RX path uses back_lock */
 	spin_lock_bh(&conn->session->back_lock);
 	if (cleanup_queued_task(task)) {
 		spin_unlock_bh(&conn->session->back_lock);
@@ -626,6 +615,7 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
 	else
 		state = ISCSI_TASK_ABRT_TMF;
 
+	sc = task->sc;
 	sc->result = err << 16;
 	scsi_set_resid(sc, scsi_bufflen(sc));
 	iscsi_complete_task(task, state);
@@ -1893,27 +1883,39 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
 }
 
 /*
- * Fail commands. session lock held and recv side suspended and xmit
- * thread flushed
+ * Fail commands. session frwd lock held and xmit thread flushed.
  */
 static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
 {
+	struct iscsi_session *session = conn->session;
 	struct iscsi_task *task;
 	int i;
 
-	for (i = 0; i < conn->session->cmds_max; i++) {
-		task = conn->session->cmds[i];
+	spin_lock_bh(&session->back_lock);
+	for (i = 0; i < session->cmds_max; i++) {
+		task = session->cmds[i];
 		if (!task->sc || task->state == ISCSI_TASK_FREE)
 			continue;
 
 		if (lun != -1 && lun != task->sc->device->lun)
 			continue;
 
-		ISCSI_DBG_SESSION(conn->session,
+		__iscsi_get_task(task);
+		spin_unlock_bh(&session->back_lock);
+
+		ISCSI_DBG_SESSION(session,
 				  "failing sc %p itt 0x%x state %d\n",
 				  task->sc, task->itt, task->state);
 		fail_scsi_task(task, error);
+
+		spin_unlock_bh(&session->frwd_lock);
+		iscsi_put_task(task);
+		spin_lock_bh(&session->frwd_lock);
+
+		spin_lock_bh(&session->back_lock);
 	}
+
+	spin_unlock_bh(&session->back_lock);
 }
 
 /**
@@ -1991,6 +1993,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 	ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
 
 	spin_lock_bh(&session->frwd_lock);
+	spin_lock(&session->back_lock);
 	task = (struct iscsi_task *)sc->SCp.ptr;
 	if (!task) {
 		/*
@@ -1998,8 +2001,11 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 		 * so let timeout code complete it now.
 		 */
 		rc = BLK_EH_DONE;
+		spin_unlock(&session->back_lock);
 		goto done;
 	}
+	__iscsi_get_task(task);
+	spin_unlock(&session->back_lock);
 
 	if (session->state != ISCSI_STATE_LOGGED_IN) {
 		/*
@@ -2058,6 +2064,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 		goto done;
 	}
 
+	spin_lock(&session->back_lock);
 	for (i = 0; i < conn->session->cmds_max; i++) {
 		running_task = conn->session->cmds[i];
 		if (!running_task->sc || running_task == task ||
@@ -2090,10 +2097,12 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 				     "last xfer %lu/%lu. Last check %lu.\n",
 				     task->last_xfer, running_task->last_xfer,
 				     task->last_timeout);
+			spin_unlock(&session->back_lock);
 			rc = BLK_EH_RESET_TIMER;
 			goto done;
 		}
 	}
+	spin_unlock(&session->back_lock);
 
 	/* Assumes nop timeout is shorter than scsi cmd timeout */
 	if (task->have_checked_conn)
@@ -2115,9 +2124,12 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 	rc = BLK_EH_RESET_TIMER;
 
 done:
-	if (task)
-		task->last_timeout = jiffies;
 	spin_unlock_bh(&session->frwd_lock);
+
+	if (task) {
+		task->last_timeout = jiffies;
+		iscsi_put_task(task);
+	}
 	ISCSI_DBG_EH(session, "return %s\n", rc == BLK_EH_RESET_TIMER ?
 		     "timer reset" : "shutdown or nh");
 	return rc;
@@ -2225,15 +2237,20 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 	conn->eh_abort_cnt++;
 	age = session->age;
 
+	spin_lock(&session->back_lock);
 	task = (struct iscsi_task *)sc->SCp.ptr;
-	ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n",
-		     sc, task->itt);
-
-	/* task completed before time out */
-	if (!task->sc) {
+	if (!task || !task->sc) {
+		/* task completed before time out */
 		ISCSI_DBG_EH(session, "sc completed while abort in progress\n");
-		goto success;
+
+		spin_unlock(&session->back_lock);
+		spin_unlock_bh(&session->frwd_lock);
+		mutex_unlock(&session->eh_mutex);
+		return SUCCESS;
 	}
+	ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
+	__iscsi_get_task(task);
+	spin_unlock(&session->back_lock);
 
 	if (task->state == ISCSI_TASK_PENDING) {
 		fail_scsi_task(task, DID_ABORT);
@@ -2295,6 +2312,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 success_unlocked:
 	ISCSI_DBG_EH(session, "abort success [sc %p itt 0x%x]\n",
 		     sc, task->itt);
+	iscsi_put_task(task);
 	mutex_unlock(&session->eh_mutex);
 	return SUCCESS;
 
@@ -2303,6 +2321,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
 failed_unlocked:
 	ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
 		     task ? task->itt : 0);
+	iscsi_put_task(task);
 	mutex_unlock(&session->eh_mutex);
 	return FAILED;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-02-07  4:48 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03  1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie
2021-02-03  1:33 ` [PATCH 1/9] libiscsi: fix iscsi_prep_scsi_cmd_pdu error handling Mike Christie
2021-02-03  1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie
2021-02-03 10:19   ` Dan Carpenter
2021-02-03 10:19     ` Dan Carpenter
2021-02-03 10:19     ` Dan Carpenter
2021-02-03 17:10     ` Mike Christie
2021-02-03 17:10       ` Mike Christie
2021-02-03  1:33 ` [PATCH 3/9] libiscsi: fix iscsi_task use after free Mike Christie
2021-02-03  1:33 ` [PATCH 4/9] libiscsi: fix iscsi host workq destruction Mike Christie
2021-02-03  1:33 ` [PATCH 5/9] libiscsi: add helper to calc max scsi cmds per session Mike Christie
2021-02-03  1:33 ` [PATCH 6/9] iscsi_tcp: fix shost can_queue initialization Mike Christie
2021-02-03 23:33   ` Lee Duncan
2021-02-03  1:33 ` [PATCH 7/9] libiscsi: reset max/exp cmdsn during recovery Mike Christie
2021-02-03  1:33 ` [PATCH 8/9] qla4xxx: use iscsi_is_session_online Mike Christie
2021-02-03  1:33 ` [PATCH 9/9] iscsi class: drop session lock in iscsi_session_chkready Mike Christie
2021-02-07  4:45 [PATCH 0/9 V6] iscsi fixes and cleanups Mike Christie
2021-02-07  4:46 ` [PATCH 3/9] libiscsi: fix iscsi_task use after free Mike Christie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.