[PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Bart Van Assche]

For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
transport_generic_free_cmd() causes RDMA completion processing to stall.
Hence only sleep inside this function if the (iSCSI) target driver
requires this.

This patch avoids that messages similar to the following appear in the
kernel log:

INFO: task kworker/u25:0:1013 blocked for more than 480 seconds.
      Tainted: G        W       4.10.0-rc7-dbg+ #3
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u25:0   D    0  1013      2 0x00000000
Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
Call Trace:
 __schedule+0x2da/0xb00
 schedule+0x38/0x90
 schedule_timeout+0x2fe/0x640
 wait_for_completion+0xfe/0x160
 transport_generic_free_cmd+0x2e/0x80 [target_core_mod]
 srpt_send_done+0x59/0x9f [ib_srpt]
 __ib_process_cq+0x4b/0xd0 [ib_core]
 ib_cq_poll_work+0x1b/0x60 [ib_core]
 process_one_work+0x208/0x6a0
 worker_thread+0x49/0x4a0
 kthread+0x107/0x140
 ret_from_fork+0x2e/0x40

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/target_core_transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 37f57357d4a0..df1ceb2dd110 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2553,7 +2553,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
 	 */
 	if (aborted) {
 		pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
-		wait_for_completion(&cmd->cmd_wait_comp);
+		if (wait_for_tasks)
+			wait_for_completion(&cmd->cmd_wait_comp);
 		cmd->se_tfo->release_cmd(cmd);
 		ret = 1;
 	}
-- 
2.12.2

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Hannes Reinecke]

On 05/05/2017 12:50 AM, Bart Van Assche wrote:
> For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> transport_generic_free_cmd() causes RDMA completion processing to stall.
> Hence only sleep inside this function if the (iSCSI) target driver
> requires this.
> 
> This patch avoids that messages similar to the following appear in the
> kernel log:
> 
> INFO: task kworker/u25:0:1013 blocked for more than 480 seconds.
>       Tainted: G        W       4.10.0-rc7-dbg+ #3
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u25:0   D    0  1013      2 0x00000000
> Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> Call Trace:
>  __schedule+0x2da/0xb00
>  schedule+0x38/0x90
>  schedule_timeout+0x2fe/0x640
>  wait_for_completion+0xfe/0x160
>  transport_generic_free_cmd+0x2e/0x80 [target_core_mod]
>  srpt_send_done+0x59/0x9f [ib_srpt]
>  __ib_process_cq+0x4b/0xd0 [ib_core]
>  ib_cq_poll_work+0x1b/0x60 [ib_core]
>  process_one_work+0x208/0x6a0
>  worker_thread+0x49/0x4a0
>  kthread+0x107/0x140
>  ret_from_fork+0x2e/0x40
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_transport.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Christoph Hellwig]

On Thu, May 04, 2017 at 03:50:46PM -0700, Bart Van Assche wrote:
> For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> transport_generic_free_cmd() causes RDMA completion processing to stall.
> Hence only sleep inside this function if the (iSCSI) target driver
> requires this.

This looks reasonable to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But as a further step can we try to move the waiting behavior entirely
into the caller that actually cares,

e.g. move the conditional target_wait_free_cmd before the call
to transport_generic_free_cmd in transport_generic_free_cmd,
and move this second wait_for_completion after the
transport_generic_free_cmd call based on an indicator (return value
or se_cmd flag)?

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Bart Van Assche]

On Fri, 2017-05-05 at 10:53 +0200, Christoph Hellwig wrote:
> But as a further step can we try to move the waiting behavior entirely
> into the caller that actually cares,
> 
> e.g. move the conditional target_wait_free_cmd before the call
> to transport_generic_free_cmd in transport_generic_free_cmd,
> and move this second wait_for_completion after the
> transport_generic_free_cmd call based on an indicator (return value
> or se_cmd flag)?

Hello Christoph,

That sounds like a good idea to me, especially since there are only three
target drivers that set the "wait_for_tasks" argument to true, namely
tcm_loop, the iSCSI target driver and the xen-scsiback driver.

Bart.

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Bart Van Assche]

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

On Fri, 2017-05-05 at 10:53 +0200, Christoph Hellwig wrote:
> On Thu, May 04, 2017 at 03:50:46PM -0700, Bart Van Assche wrote:
> > For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> > transport_generic_free_cmd() causes RDMA completion processing to stall.
> > Hence only sleep inside this function if the (iSCSI) target driver
> > requires this.
> 
> This looks reasonable to me:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But as a further step can we try to move the waiting behavior entirely
> into the caller that actually cares,
> 
> e.g. move the conditional target_wait_free_cmd before the call
> to transport_generic_free_cmd in transport_generic_free_cmd,
> and move this second wait_for_completion after the
> transport_generic_free_cmd call based on an indicator (return value
> or se_cmd flag)?

Hello Christoph,

I have started working on eliminating the waiting code from
transport_generic_free_cmd(). I'm still working on a patch for the iSCSI
target driver. What I came up so far for the loop and Xen scsiback drivers
is attached to this e-mail.

Bart.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xen-scsiback-Replace-a-waitqueue-and-a-counter-by-a-.patch --]
[-- Type: text/x-patch; name="0001-xen-scsiback-Replace-a-waitqueue-and-a-counter-by-a-.patch", Size: 1893 bytes --]

From 4a56d0f88e02a26aace617709a35b9ee81856890 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 10 May 2017 15:40:39 -0700
Subject: [PATCH 1/5] xen/scsiback: Replace a waitqueue and a counter by a
 completion

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: xen-devel@lists.xenproject.org
---
 drivers/xen/xen-scsiback.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index d6950e0802b7..5f1daf161312 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -137,8 +137,7 @@ struct vscsibk_pend {
 };
 
 struct scsiback_tmr {
-	atomic_t tmr_complete;
-	wait_queue_head_t tmr_wait;
+	struct completion done;
 };
 
 #define VSCSI_DEFAULT_SESSION_TAGS	128
@@ -609,7 +608,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 		goto err;
 	}
 
-	init_waitqueue_head(&tmr->tmr_wait);
+	init_completion(&tmr->done);
 
 	rc = target_submit_tmr(&pending_req->se_cmd, nexus->tvn_se_sess,
 			       &pending_req->sense_buffer[0],
@@ -618,7 +617,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	if (rc)
 		goto err;
 
-	wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
+	wait_for_completion(&tmr->done);
 
 	err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
 		SUCCESS : FAILED;
@@ -1458,8 +1457,7 @@ static void scsiback_queue_tm_rsp(struct se_cmd *se_cmd)
 	struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
 	struct scsiback_tmr *tmr = se_tmr->fabric_tmr_ptr;
 
-	atomic_set(&tmr->tmr_complete, 1);
-	wake_up(&tmr->tmr_wait);
+	complete(&tmr->done);
 }
 
 static void scsiback_aborted_task(struct se_cmd *se_cmd)
-- 
2.12.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-xen-scsiback-Make-TMF-processing-slightly-faster.patch --]
[-- Type: text/x-patch; name="0002-xen-scsiback-Make-TMF-processing-slightly-faster.patch", Size: 1529 bytes --]

From 043951a899f826a9a20733cd18006dbb7977e6b5 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 10 May 2017 15:13:04 -0700
Subject: [PATCH 2/5] xen/scsiback: Make TMF processing slightly faster

Target drivers must guarantee that struct se_cmd and struct se_tmr_req
exist as long as target_tmr_work() is in progress. Since the last
access by the LIO core is a call to .check_stop_free() and since the
Xen scsiback .check_stop_free() drops a reference to the TMF, it is
already guaranteed that the struct se_cmd that corresponds to the TMF
exists as long as target_tmr_work() is in progress. Hence change the
second argument of transport_generic_free_cmd() from 1 into 0.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: xen-devel@lists.xenproject.org
---
 drivers/xen/xen-scsiback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 5f1daf161312..3a67acb33dfb 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -623,7 +623,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 		SUCCESS : FAILED;
 
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
-	transport_generic_free_cmd(&pending_req->se_cmd, 1);
+	transport_generic_free_cmd(&pending_req->se_cmd, 0);
 	return;
 err:
 	if (tmr)
-- 
2.12.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-target-tcm_loop-Replace-a-waitqueue-and-a-counter-by.patch --]
[-- Type: text/x-patch; name="0003-target-tcm_loop-Replace-a-waitqueue-and-a-counter-by.patch", Size: 2527 bytes --]

From 901c4cd1f1694fbe2edf235734462056ad58a9e8 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 10 May 2017 14:35:55 -0700
Subject: [PATCH 3/5] target/tcm_loop: Replace a waitqueue and a counter by a
 completion

Make the tcm_loop code slightly easier to read by using a struct
completion instead of open-coding the completion concept.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
---
 drivers/target/loopback/tcm_loop.c | 13 +++++--------
 drivers/target/loopback/tcm_loop.h |  3 +--
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 5091b31b3e56..25b6cab5e158 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -245,7 +245,7 @@ static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 		pr_err("Unable to allocate memory for tl_tmr\n");
 		goto release;
 	}
-	init_waitqueue_head(&tl_tmr->tl_tmr_wait);
+	init_completion(&tl_tmr->done);
 
 	se_cmd = &tl_cmd->tl_se_cmd;
 	se_tpg = &tl_tpg->tl_se_tpg;
@@ -276,7 +276,7 @@ static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 	 * tcm_loop_queue_tm_rsp() to wake us up.
 	 */
 	transport_generic_handle_tmr(se_cmd);
-	wait_event(tl_tmr->tl_tmr_wait, atomic_read(&tl_tmr->tmr_complete));
+	wait_for_completion(&tl_tmr->done);
 	/*
 	 * The TMR LUN_RESET has completed, check the response status and
 	 * then release allocations.
@@ -671,12 +671,9 @@ static void tcm_loop_queue_tm_rsp(struct se_cmd *se_cmd)
 {
 	struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
 	struct tcm_loop_tmr *tl_tmr = se_tmr->fabric_tmr_ptr;
-	/*
-	 * The SCSI EH thread will be sleeping on se_tmr->tl_tmr_wait, go ahead
-	 * and wake up the wait_queue_head_t in tcm_loop_device_reset()
-	 */
-	atomic_set(&tl_tmr->tmr_complete, 1);
-	wake_up(&tl_tmr->tl_tmr_wait);
+
+	/* Wake up tcm_loop_issue_tmr(). */
+	complete(&tl_tmr->done);
 }
 
 static void tcm_loop_aborted_task(struct se_cmd *se_cmd)
diff --git a/drivers/target/loopback/tcm_loop.h b/drivers/target/loopback/tcm_loop.h
index a8a230b4e6b5..f54b605c33c4 100644
--- a/drivers/target/loopback/tcm_loop.h
+++ b/drivers/target/loopback/tcm_loop.h
@@ -21,8 +21,7 @@ struct tcm_loop_cmd {
 };
 
 struct tcm_loop_tmr {
-	atomic_t tmr_complete;
-	wait_queue_head_t tl_tmr_wait;
+	struct completion done;
 };
 
 struct tcm_loop_nexus {
-- 
2.12.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-target-tcm_loop-Simplify-the-task-management-functio.patch --]
[-- Type: text/x-patch; name="0004-target-tcm_loop-Simplify-the-task-management-functio.patch", Size: 2547 bytes --]

From 1b09afac4c85a423f24397de43ee4582d0d1aac7 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 10 May 2017 14:30:11 -0700
Subject: [PATCH 4/5] target/tcm_loop: Simplify the task management function
 implementation

Use target_submit_tmr() instead of open-coding this function. The
only functional change is that TMFs are now added to sess_cmd_list,
something the current code does not do.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
---
 drivers/target/loopback/tcm_loop.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 25b6cab5e158..48d751c53104 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -218,7 +218,6 @@ static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 {
 	struct se_cmd *se_cmd = NULL;
 	struct se_session *se_sess;
-	struct se_portal_group *se_tpg;
 	struct tcm_loop_nexus *tl_nexus;
 	struct tcm_loop_cmd *tl_cmd = NULL;
 	struct tcm_loop_tmr *tl_tmr = NULL;
@@ -248,40 +247,16 @@ static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 	init_completion(&tl_tmr->done);
 
 	se_cmd = &tl_cmd->tl_se_cmd;
-	se_tpg = &tl_tpg->tl_se_tpg;
 	se_sess = tl_tpg->tl_nexus->se_sess;
-	/*
-	 * Initialize struct se_cmd descriptor from target_core_mod infrastructure
-	 */
-	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, 0,
-				DMA_NONE, TCM_SIMPLE_TAG,
-				&tl_cmd->tl_sense_buf[0]);
 
-	rc = core_tmr_alloc_req(se_cmd, tl_tmr, tmr, GFP_KERNEL);
+	rc = target_submit_tmr(se_cmd, se_sess, NULL, 0 /* unpacked_lun */,
+			       tl_tmr, tmr, GFP_KERNEL, TCM_SIMPLE_TAG,
+			       0 /*flags*/);
 	if (rc < 0)
 		goto release;
-
-	if (tmr == TMR_ABORT_TASK)
-		se_cmd->se_tmr_req->ref_task_tag = task;
-
-	/*
-	 * Locate the underlying TCM struct se_lun
-	 */
-	if (transport_lookup_tmr_lun(se_cmd, lun) < 0) {
-		ret = TMR_LUN_DOES_NOT_EXIST;
-		goto release;
-	}
-	/*
-	 * Queue the TMR to TCM Core and sleep waiting for
-	 * tcm_loop_queue_tm_rsp() to wake us up.
-	 */
-	transport_generic_handle_tmr(se_cmd);
 	wait_for_completion(&tl_tmr->done);
-	/*
-	 * The TMR LUN_RESET has completed, check the response status and
-	 * then release allocations.
-	 */
 	ret = se_cmd->se_tmr_req->response;
+
 release:
 	if (se_cmd)
 		transport_generic_free_cmd(se_cmd, 1);
-- 
2.12.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-target-tcm_loop-Make-TMF-processing-slightly-faster.patch --]
[-- Type: text/x-patch; name="0005-target-tcm_loop-Make-TMF-processing-slightly-faster.patch", Size: 2900 bytes --]

From 4c961dc45bb5d35cdefa8f0f94ebd8453a9d6cc4 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 10 May 2017 15:01:38 -0700
Subject: [PATCH 5/5] target/tcm_loop: Make TMF processing slightly faster

Target drivers must guarantee that struct se_cmd and struct se_tmr_req
exist as long as target_tmr_work() is in progress. This is why the
tcm_loop driver today passes 1 as second argument to
transport_generic_free_cmd() from inside the TMF code. Instead of
making the TMF code wait, make the TMF code obtain two references
(SCF_ACK_KREF) and drop one reference from inside the .check_stop_free()
callback.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
---
 drivers/target/loopback/tcm_loop.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 48d751c53104..136d70285f9a 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -51,27 +51,18 @@ static int tcm_loop_queue_status(struct se_cmd *se_cmd);
  */
 static int tcm_loop_check_stop_free(struct se_cmd *se_cmd)
 {
-	/*
-	 * Do not release struct se_cmd's containing a valid TMR
-	 * pointer.  These will be released directly in tcm_loop_device_reset()
-	 * with transport_generic_free_cmd().
-	 */
-	if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
-		return 0;
-	/*
-	 * Release the struct se_cmd, which will make a callback to release
-	 * struct tcm_loop_cmd * in tcm_loop_deallocate_core_cmd()
-	 */
-	transport_generic_free_cmd(se_cmd, 0);
-	return 1;
+	return transport_generic_free_cmd(se_cmd, 0);
 }
 
 static void tcm_loop_release_cmd(struct se_cmd *se_cmd)
 {
 	struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
 				struct tcm_loop_cmd, tl_se_cmd);
+	struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
+	struct tcm_loop_tmr *tl_tmr = se_tmr ? se_tmr->fabric_tmr_ptr : NULL;
 
 	kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
+	kfree(tl_tmr);
 }
 
 static int tcm_loop_show_info(struct seq_file *m, struct Scsi_Host *host)
@@ -251,19 +242,23 @@ static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 
 	rc = target_submit_tmr(se_cmd, se_sess, NULL, 0 /* unpacked_lun */,
 			       tl_tmr, tmr, GFP_KERNEL, TCM_SIMPLE_TAG,
-			       0 /*flags*/);
+			       TARGET_SCF_ACK_KREF);
 	if (rc < 0)
 		goto release;
 	wait_for_completion(&tl_tmr->done);
 	ret = se_cmd->se_tmr_req->response;
+	target_put_sess_cmd(se_cmd);
+
+out:
+	return ret;
 
 release:
 	if (se_cmd)
-		transport_generic_free_cmd(se_cmd, 1);
+		transport_generic_free_cmd(se_cmd, 0);
 	else
 		kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
 	kfree(tl_tmr);
-	return ret;
+	goto out;
 }
 
 static int tcm_loop_abort_task(struct scsi_cmnd *sc)
-- 
2.12.2

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Nicholas A. Bellinger]

On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> transport_generic_free_cmd() causes RDMA completion processing to stall.
> Hence only sleep inside this function if the (iSCSI) target driver
> requires this.
> 
> This patch avoids that messages similar to the following appear in the
> kernel log:
> 
> INFO: task kworker/u25:0:1013 blocked for more than 480 seconds.
>       Tainted: G        W       4.10.0-rc7-dbg+ #3
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u25:0   D    0  1013      2 0x00000000
> Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> Call Trace:
>  __schedule+0x2da/0xb00
>  schedule+0x38/0x90
>  schedule_timeout+0x2fe/0x640
>  wait_for_completion+0xfe/0x160
>  transport_generic_free_cmd+0x2e/0x80 [target_core_mod]
>  srpt_send_done+0x59/0x9f [ib_srpt]
>  __ib_process_cq+0x4b/0xd0 [ib_core]
>  ib_cq_poll_work+0x1b/0x60 [ib_core]
>  process_one_work+0x208/0x6a0
>  worker_thread+0x49/0x4a0
>  kthread+0x107/0x140
>  ret_from_fork+0x2e/0x40
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_transport.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 37f57357d4a0..df1ceb2dd110 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2553,7 +2553,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
>  	 */
>  	if (aborted) {
>  		pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
> -		wait_for_completion(&cmd->cmd_wait_comp);
> +		if (wait_for_tasks)
> +			wait_for_completion(&cmd->cmd_wait_comp);
>  		cmd->se_tfo->release_cmd(cmd);
>  		ret = 1;
>  	}

Grr, we have already been through this once before, remember..?

http://www.spinics.net/lists/target-devel/msg14598.html

To repeat, aborted = true is only set when transport_generic_free_cmd()
is called with wait_for_tasks = true, and neither srpt nor isert have
ever invoked transport_generic_free_cmd() with wait_for_tasks = true in
upstream code:

drivers/infiniband/ulp/isert/ib_isert.c:		transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/isert/ib_isert.c:		transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/isert/ib_isert.c:			transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c:		transport_generic_free_cmd(&ioctx->cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c:		transport_generic_free_cmd(&ioctx->cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c:		transport_generic_free_cmd(&ioctx->cmd, 0);

And even if they did, this patch still would be a NOP and not make any
difference !

So it's clear what you did here, once upon a time you came up with a
bogus patch to address breakage for you own stuff in out-of-tree code,
and then copy-and-pasted the old backtrace from the out-of-tree bug and
posted it as an fix here, for a scenario that can't ever possibly occur
in upstream.

Really, please stop sending this type of garbage as it further erodes
what little trust I have left.

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Bart Van Assche]

On Sun, 2017-05-07 at 15:20 -0700, Nicholas A. Bellinger wrote:
> aborted = true is only set when transport_generic_free_cmd()
> is called with wait_for_tasks = true, and neither srpt nor isert have
> ever invoked transport_generic_free_cmd() with wait_for_tasks = true in
> upstream code.

Right. Although this patch is not wrong, this patch is not needed with the
current transport_generic_free_cmd() implementation but has to be folded in
the patch that eliminates the "aborted" and "tas" variables (which is not
in the series I posted).

Bart.

Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

[Nicholas A. Bellinger]

On Mon, 2017-05-08 at 21:25 +0000, Bart Van Assche wrote:
> On Sun, 2017-05-07 at 15:20 -0700, Nicholas A. Bellinger wrote:
> > aborted = true is only set when transport_generic_free_cmd()
> > is called with wait_for_tasks = true, and neither srpt nor isert have
> > ever invoked transport_generic_free_cmd() with wait_for_tasks = true in
> > upstream code.
> 
> Right. Although this patch is not wrong, this patch is not needed with the
> current transport_generic_free_cmd() implementation but has to be folded in
> the patch that eliminates the "aborted" and "tas" variables (which is not
> in the series I posted).

I don't give a toss if the patch is logically a NOP, and 'is not wrong'.

The commit message was completely bogus, and the backtrace was from some
random out-of-tree junk that was obviously not tested against the
current target-pending/for-next.

And to top it off, I've already pointed out these facts three months
ago, and you just ignored the response.

The point is, it's sloppy and means I have to repeat myself over and
over again.

Not to mention in that very same thread from three months ago, I asked
(again( to stop mixing bug-fixes with random improvements:

http://www.spinics.net/lists/target-devel/msg14665.html

"To reiterate the importance of having bug-fixes, especially those
intended for stable, always be leading other patches..

There is no way for a maintainer to know which bug-fixes are to existing
code unless they precede all other patches and not intermixed with
various other changes.  The bug-fixes to existing upstream code need to
be tested on their own without the other changes (especially those that
effect the same area) invalidating the tests."

How many more times will I have to repeat myself before it sinks in..?

[PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang

[Bart Van Assche]

>From __target_execute_cmd() (simplified):

	ret = cmd->execute_cmd(cmd);
	if (ret != 0)
		transport_generic_request_failure(cmd, ret);

>From sbc_execute_rw():

	return ops->execute_rw(cmd, cmd->t_data_sg, cmd->t_data_nents,
			       cmd->data_direction);

This means that sbc_ops.execute_rw() must either return a non-zero
value or call target_complete_cmd() and return zero or command
execution stalls. Make sure that fd_execute_rw() calls
target_complete_cmd() even for zero-length commands.

Fixes: commit c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/target_core_file.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 1bf6c31e4c21..73b8f93a5fef 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -608,8 +608,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	if (ret < 0)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-	if (ret)
-		target_complete_cmd(cmd, SAM_STAT_GOOD);
+	target_complete_cmd(cmd, SAM_STAT_GOOD);
 	return 0;
 }
 
-- 
2.12.2

Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang

[Hannes Reinecke]

On 05/05/2017 12:50 AM, Bart Van Assche wrote:
> From __target_execute_cmd() (simplified):
> 
> 	ret = cmd->execute_cmd(cmd);
> 	if (ret != 0)
> 		transport_generic_request_failure(cmd, ret);
> 
> From sbc_execute_rw():
> 
> 	return ops->execute_rw(cmd, cmd->t_data_sg, cmd->t_data_nents,
> 			       cmd->data_direction);
> 
> This means that sbc_ops.execute_rw() must either return a non-zero
> value or call target_complete_cmd() and return zero or command
> execution stalls. Make sure that fd_execute_rw() calls
> target_complete_cmd() even for zero-length commands.
> 
> Fixes: commit c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_file.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang

[Nicholas A. Bellinger]

On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> From __target_execute_cmd() (simplified):
> 
> 	ret = cmd->execute_cmd(cmd);
> 	if (ret != 0)
> 		transport_generic_request_failure(cmd, ret);
> 
> From sbc_execute_rw():
> 
> 	return ops->execute_rw(cmd, cmd->t_data_sg, cmd->t_data_nents,
> 			       cmd->data_direction);
> 
> This means that sbc_ops.execute_rw() must either return a non-zero
> value or call target_complete_cmd() and return zero or command
> execution stalls. Make sure that fd_execute_rw() calls
> target_complete_cmd() even for zero-length commands.
> 
> Fixes: commit c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_file.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 1bf6c31e4c21..73b8f93a5fef 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -608,8 +608,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>  	if (ret < 0)
>  		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  
> -	if (ret)
> -		target_complete_cmd(cmd, SAM_STAT_GOOD);
> +	target_complete_cmd(cmd, SAM_STAT_GOOD);
>  	return 0;
>  }
>  

Applied, with a commit log to reflect that zero-length commands used to
be handled by target-core, but since commit d81cb447 (v3.7) they are now
submitted to backend drivers.

[PATCH 05/19] target: Allocate sg-list correctly

[Bart Van Assche]

Avoid that the iSCSI target driver complains about "Initial page entry
out-of-bounds" and closes the connection if the SCSI Data-Out buffer
is larger than the buffer size specified through the CDB. This patch
prevents that running the libiscsi regression tests against LIO trigger
an infinite loop of libiscsi submitting a command, LIO closing the
connection and libiscsi resubmitting the same command.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/target_core_transport.c | 29 ++++++++++-------------------
 include/target/target_core_base.h      |  4 +++-
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index df1ceb2dd110..86b6b0238975 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1159,11 +1159,11 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 
 	if (cmd->unknown_data_length) {
 		cmd->data_length = size;
-	} else if (size != cmd->data_length) {
+	} else if (size != cmd->buffer_length) {
 		pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
 			" %u does not match SCSI CDB Length: %u for SAM Opcode:"
 			" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
-				cmd->data_length, size, cmd->t_task_cdb[0]);
+			cmd->buffer_length, size, cmd->t_task_cdb[0]);
 
 		if (cmd->data_direction == DMA_TO_DEVICE &&
 		    cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
@@ -1183,7 +1183,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 		}
 		/*
 		 * For the overflow case keep the existing fabric provided
-		 * ->data_length.  Otherwise for the underflow case, reset
+		 * ->buffer_length.  Otherwise for the underflow case, reset
 		 * ->data_length to the smaller SCSI expected data transfer
 		 * length.
 		 */
@@ -1227,6 +1227,7 @@ void transport_init_se_cmd(
 
 	cmd->se_tfo = tfo;
 	cmd->se_sess = se_sess;
+	cmd->buffer_length = data_length;
 	cmd->data_length = data_length;
 	cmd->data_direction = data_direction;
 	cmd->sam_task_attr = task_attr;
@@ -2412,41 +2413,31 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 	 * beforehand.
 	 */
 	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) &&
-	    cmd->data_length) {
+	    cmd->buffer_length) {
 
 		if ((cmd->se_cmd_flags & SCF_BIDI) ||
 		    (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) {
-			u32 bidi_length;
-
-			if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)
-				bidi_length = cmd->t_task_nolb *
-					      cmd->se_dev->dev_attrib.block_size;
-			else
-				bidi_length = cmd->data_length;
-
 			ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
 					       &cmd->t_bidi_data_nents,
-					       bidi_length, zero_flag, false);
+					       cmd->buffer_length, zero_flag,
+					       false);
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 
 		ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
-				       cmd->data_length, zero_flag, false);
+				       cmd->buffer_length, zero_flag, false);
 		if (ret < 0)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	} else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) &&
-		    cmd->data_length) {
+		    cmd->buffer_length) {
 		/*
 		 * Special case for COMPARE_AND_WRITE with fabrics
 		 * using SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC.
 		 */
-		u32 caw_length = cmd->t_task_nolb *
-				 cmd->se_dev->dev_attrib.block_size;
-
 		ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
 				       &cmd->t_bidi_data_nents,
-				       caw_length, zero_flag, false);
+				       cmd->buffer_length, zero_flag, false);
 		if (ret < 0)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 49cd03c2d943..0660585cb03d 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -455,7 +455,9 @@ struct se_cmd {
 	enum transport_state_table t_state;
 	/* See se_cmd_flags_table */
 	u32			se_cmd_flags;
-	/* Total size in bytes associated with command */
+	/* Length of Data-Out or Data-In buffer */
+	u32			buffer_length;
+	/* Number of bytes affected on storage medium */
 	u32			data_length;
 	u32			residual_count;
 	u64			orig_fe_lun;
-- 
2.12.2

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Hannes Reinecke]

On 05/05/2017 12:50 AM, Bart Van Assche wrote:
> Avoid that the iSCSI target driver complains about "Initial page entry
> out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> is larger than the buffer size specified through the CDB. This patch
> prevents that running the libiscsi regression tests against LIO trigger
> an infinite loop of libiscsi submitting a command, LIO closing the
> connection and libiscsi resubmitting the same command.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_transport.c | 29 ++++++++++-------------------
>  include/target/target_core_base.h      |  4 +++-
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Christoph Hellwig]

On Thu, May 04, 2017 at 03:50:48PM -0700, Bart Van Assche wrote:
> Avoid that the iSCSI target driver complains about "Initial page entry
> out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> is larger than the buffer size specified through the CDB. This patch
> prevents that running the libiscsi regression tests against LIO trigger
> an infinite loop of libiscsi submitting a command, LIO closing the
> connection and libiscsi resubmitting the same command.

Can you add a bit more of an explanation of why this happens?  I've
just tried to verify the area, but at least while sitting in a conference
talk I can't quite make sense of the changes.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Bart Van Assche]

On Fri, 2017-05-05 at 11:06 +0200, Christoph Hellwig wrote:
> On Thu, May 04, 2017 at 03:50:48PM -0700, Bart Van Assche wrote:
> > Avoid that the iSCSI target driver complains about "Initial page entry
> > out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> > is larger than the buffer size specified through the CDB. This patch
> > prevents that running the libiscsi regression tests against LIO trigger
> > an infinite loop of libiscsi submitting a command, LIO closing the
> > connection and libiscsi resubmitting the same command.
> 
> Can you add a bit more of an explanation of why this happens?  I've
> just tried to verify the area, but at least while sitting in a conference
> talk I can't quite make sense of the changes.

Hello Christoph,

The aspects of SCSI command processing that are relevant in this context are:
* When the iSCSI target driver receives a SCSI command it calls
  transport_init_se_cmd() to initialize struct se_cmd. The iSCSI target driver
  passes the number of bytes that will be transferred into the "data_length"
  argument of transport_init_se_cmd(). That function stores the data length in
  the .data_length member of struct se_cmd. The value passed by target drivers
  to transport_init_se_cmd() is what is called the Expected Data Transfer
  Length (EDTL) in the iSCSI RFC.
* After CDB parsing has finished target_cmd_size_check() is called. If EDTL
  exceeds the data buffer size extracted from the SCSI CDB (CDBL) then
  .data_length is reduced to CDBL.
* Next target_alloc_sgl() allocates an sg-list for .data_length bytes (CDBL).
* iscsit_allocate_iovecs() allocates a struct kvec (.iov_data) also for
  .data_length bytes (CDBL).
* iscsit_get_dataout() calls rx_data() and tries to store EDTL bytes in the
  allocated struct kvec. If EDTL > CDBL then .iov_data overflows and this
  usually triggers a crash. With the patch that prevents .iov_data overflows
  the initiator is disconnected. In the case of libiscsi, it keeps retrying
  forever to resubmit SCSI commands for which EDTL > CDBL.

In other words, initially EDTL is stored into .data_length and later on the
value of .data_length changes to CDBL. My proposal to avoid the buffer
overflows is to store both EDTL and CDBL in struct se_cmd and to allocate an
sg-list per command that can store EDTL bytes instead of CDBL bytes.

Bart.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Nicholas A. Bellinger]

On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> Avoid that the iSCSI target driver complains about "Initial page entry
> out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> is larger than the buffer size specified through the CDB. This patch
> prevents that running the libiscsi regression tests against LIO trigger
> an infinite loop of libiscsi submitting a command, LIO closing the
> connection and libiscsi resubmitting the same command.
> 

This statement and patch is incorrect.

target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with
overflow/underflow since day one:

>From target_cmd_size_check():

        } else if (size != cmd->data_length) {
                pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
                        " %u does not match SCSI CDB Length: %u for SAM Opcode:"
                        " 0x%02x\n", cmd->se_tfo->get_fabric_name(),
                                cmd->data_length, size, cmd->t_task_cdb[0]);

                if (cmd->data_direction == DMA_TO_DEVICE &&
                    cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
                        pr_err("Rejecting underflow/overflow WRITE data\n");
                        return TCM_INVALID_CDB_FIELD;
                }

                ......
        }

The reason you're suddenly hitting this now is because your patch in
target-pending/for-next:

   commit 0e2eb7d12eaa8e391bf5615d4271bb87a649caaa
   Author: Bart Van Assche <bart.vanassche@sandisk.com>
   Date:   Thu Mar 30 10:12:39 2017 -0700

incorrectly started allowing WRITE_VERIFY_* w/ bytchk = 0, without
actually setting SCF_SCSI_DATA_CDB in sbc_parse_verify():

        switch (bytchk) {
        case 0:
                *bufflen = 0;
                break;
        case 1:
                *bufflen = sbc_get_size(cmd, *sectors);
                cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
                break;
        default:
                pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n",
                       bytchk, cdb[0]);
                return TCM_INVALID_CDB_FIELD;
        }

Patch #18 is also trying to (incorrectly) address the same problem.

I'll post the proper fix for the regression introduced by your earlier
patch there.

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_transport.c | 29 ++++++++++-------------------
>  include/target/target_core_base.h      |  4 +++-
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index df1ceb2dd110..86b6b0238975 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1159,11 +1159,11 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
>  
>  	if (cmd->unknown_data_length) {
>  		cmd->data_length = size;
> -	} else if (size != cmd->data_length) {
> +	} else if (size != cmd->buffer_length) {
>  		pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
>  			" %u does not match SCSI CDB Length: %u for SAM Opcode:"
>  			" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
> -				cmd->data_length, size, cmd->t_task_cdb[0]);
> +			cmd->buffer_length, size, cmd->t_task_cdb[0]);
>  
>  		if (cmd->data_direction == DMA_TO_DEVICE &&
>  		    cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
> @@ -1183,7 +1183,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
>  		}
>  		/*
>  		 * For the overflow case keep the existing fabric provided
> -		 * ->data_length.  Otherwise for the underflow case, reset
> +		 * ->buffer_length.  Otherwise for the underflow case, reset
>  		 * ->data_length to the smaller SCSI expected data transfer
>  		 * length.
>  		 */
> @@ -1227,6 +1227,7 @@ void transport_init_se_cmd(
>  
>  	cmd->se_tfo = tfo;
>  	cmd->se_sess = se_sess;
> +	cmd->buffer_length = data_length;
>  	cmd->data_length = data_length;
>  	cmd->data_direction = data_direction;
>  	cmd->sam_task_attr = task_attr;
> @@ -2412,41 +2413,31 @@ transport_generic_new_cmd(struct se_cmd *cmd)
>  	 * beforehand.
>  	 */
>  	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) &&
> -	    cmd->data_length) {
> +	    cmd->buffer_length) {
>  
>  		if ((cmd->se_cmd_flags & SCF_BIDI) ||
>  		    (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) {
> -			u32 bidi_length;
> -
> -			if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)
> -				bidi_length = cmd->t_task_nolb *
> -					      cmd->se_dev->dev_attrib.block_size;
> -			else
> -				bidi_length = cmd->data_length;
> -
>  			ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
>  					       &cmd->t_bidi_data_nents,
> -					       bidi_length, zero_flag, false);
> +					       cmd->buffer_length, zero_flag,
> +					       false);
>  			if (ret < 0)
>  				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		}
>  
>  		ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
> -				       cmd->data_length, zero_flag, false);
> +				       cmd->buffer_length, zero_flag, false);
>  		if (ret < 0)
>  			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  	} else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) &&
> -		    cmd->data_length) {
> +		    cmd->buffer_length) {
>  		/*
>  		 * Special case for COMPARE_AND_WRITE with fabrics
>  		 * using SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC.
>  		 */
> -		u32 caw_length = cmd->t_task_nolb *
> -				 cmd->se_dev->dev_attrib.block_size;
> -
>  		ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
>  				       &cmd->t_bidi_data_nents,
> -				       caw_length, zero_flag, false);
> +				       cmd->buffer_length, zero_flag, false);
>  		if (ret < 0)
>  			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  	}
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 49cd03c2d943..0660585cb03d 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -455,7 +455,9 @@ struct se_cmd {
>  	enum transport_state_table t_state;
>  	/* See se_cmd_flags_table */
>  	u32			se_cmd_flags;
> -	/* Total size in bytes associated with command */
> +	/* Length of Data-Out or Data-In buffer */
> +	u32			buffer_length;
> +	/* Number of bytes affected on storage medium */
>  	u32			data_length;
>  	u32			residual_count;
>  	u64			orig_fe_lun;

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Bart Van Assche]

On Sun, 2017-05-07 at 15:45 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> > Avoid that the iSCSI target driver complains about "Initial page entry
> > out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> > is larger than the buffer size specified through the CDB. This patch
> > prevents that running the libiscsi regression tests against LIO trigger
> > an infinite loop of libiscsi submitting a command, LIO closing the
> > connection and libiscsi resubmitting the same command.
> > 
> 
> This statement and patch is incorrect.
> 
> target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with
> overflow/underflow since day one:
> 
> From target_cmd_size_check():
> 
>         } else if (size != cmd->data_length) {
>                 pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
>                         " %u does not match SCSI CDB Length: %u for SAM Opcode:"
>                         " 0x%02x\n", cmd->se_tfo->get_fabric_name(),
>                                 cmd->data_length, size, cmd->t_task_cdb[0]);
> 
>                 if (cmd->data_direction == DMA_TO_DEVICE &&
>                     cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
>                         pr_err("Rejecting underflow/overflow WRITE data\n");
>                         return TCM_INVALID_CDB_FIELD;
>                 }
> 
>                 ......
>         }

Hello Nic,

That behavior is as far as I know not compliant with the SCSI specs. In e.g.
the libiscsi test suite there are many tests that verify that a SCSI target
implementation reports OVERFLOW or UNDERFLOW where LIO today reports INVALID
FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c.
>From RFC 3720 (apparently written from the point-of-view of transfers from
target to initiator):

     bit 5 - (O) set for Residual Overflow.  In this case, the Residual
       Count indicates the number of bytes that were not transferred
       because the initiator's Expected Data Transfer Length was not
       sufficient.

     bit 6 - (U) set for Residual Underflow.  In this case, the Residual
       Count indicates the number of bytes that were not transferred out
       of the number of bytes that were expected to be transferred.

But even with the current implementation, why do you think that the reject by
target_cmd_size_check() would be sufficient to prevent the behavior I described?
>From source file iscsi_target.c:

static int
iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
			   unsigned char *buf)
{
	struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf;
	int rc, immed_data;
	bool dump_payload = false;

	rc = iscsit_setup_scsi_cmd(conn, cmd, buf);
	if (rc < 0)
		return 0;
	/*
	 * Allocation iovecs needed for struct socket operations for
	 * traditional iSCSI block I/O.
	 */
	if (iscsit_allocate_iovecs(cmd) < 0) {
		return iscsit_reject_cmd(cmd,
				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
	}
	immed_data = cmd->immediate_data;

	rc = iscsit_process_scsi_cmd(conn, cmd, hdr);
	if (rc < 0)
		return rc;
	else if (rc > 0)
		dump_payload = true;

	if (!immed_data)
		return 0;

	return iscsit_get_immediate_data(cmd, hdr, dump_payload);
}

In other words, if target_setup_cmd_from_cdb() returns a sense code (positive
value) iscsit_get_immediate_data() will call rx_data() to receive that immediate
data if there is immediate data and dump_payload == false. The code that controls
the value of dump_payload is as follows (from iscsit_process_scsi_cmd()):

	if (cmd->sense_reason) {
		if (cmd->reject_reason)
			return 0;

		return 1;
	}

In other words, if both .sense_reason and .reject_reason are set before
iscsit_process_scsi_cmd() is called then iscsit_get_immediate_data() can call
rx_data() to receive more data than what fits in the allocated buffer.

Bart.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Nicholas A. Bellinger]

On Mon, 2017-05-08 at 17:46 +0000, Bart Van Assche wrote:
> On Sun, 2017-05-07 at 15:45 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> > > Avoid that the iSCSI target driver complains about "Initial page entry
> > > out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> > > is larger than the buffer size specified through the CDB. This patch
> > > prevents that running the libiscsi regression tests against LIO trigger
> > > an infinite loop of libiscsi submitting a command, LIO closing the
> > > connection and libiscsi resubmitting the same command.
> > > 
> > 
> > This statement and patch is incorrect.
> > 
> > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with
> > overflow/underflow since day one:
> > 
> > From target_cmd_size_check():
> > 
> >         } else if (size != cmd->data_length) {
> >                 pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
> >                         " %u does not match SCSI CDB Length: %u for SAM Opcode:"
> >                         " 0x%02x\n", cmd->se_tfo->get_fabric_name(),
> >                                 cmd->data_length, size, cmd->t_task_cdb[0]);
> > 
> >                 if (cmd->data_direction == DMA_TO_DEVICE &&
> >                     cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
> >                         pr_err("Rejecting underflow/overflow WRITE data\n");
> >                         return TCM_INVALID_CDB_FIELD;
> >                 }
> > 
> >                 ......
> >         }
> 
> Hello Nic,
> 
> That behavior is as far as I know not compliant with the SCSI specs. In e.g.
> the libiscsi test suite there are many tests that verify that a SCSI target
> implementation reports OVERFLOW or UNDERFLOW where LIO today reports INVALID
> FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c.

Yes, I understand what the test does.  In practice this has never been
an issue, and we've not actually encountered a host that sends overflow
or underflow for a WRITE CDB of type SCF_SCSI_DATA_CDB.

If there is a host environment that does, I'd like to hear about it.

In any event, the point is your patch to add sbc_parse_verify() broke
existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB
assignment for all cases.

> From RFC 3720 (apparently written from the point-of-view of transfers from
> target to initiator):
> 
>      bit 5 - (O) set for Residual Overflow.  In this case, the Residual
>        Count indicates the number of bytes that were not transferred
>        because the initiator's Expected Data Transfer Length was not
>        sufficient.
> 
>      bit 6 - (U) set for Residual Underflow.  In this case, the Residual
>        Count indicates the number of bytes that were not transferred out
>        of the number of bytes that were expected to be transferred.
> 
> But even with the current implementation, why do you think that the reject by
> target_cmd_size_check() would be sufficient to prevent the behavior I described?
> From source file iscsi_target.c:
> 
> static int
> iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> 			   unsigned char *buf)
> {
> 	struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf;
> 	int rc, immed_data;
> 	bool dump_payload = false;
> 
> 	rc = iscsit_setup_scsi_cmd(conn, cmd, buf);
> 	if (rc < 0)
> 		return 0;
> 	/*
> 	 * Allocation iovecs needed for struct socket operations for
> 	 * traditional iSCSI block I/O.
> 	 */
> 	if (iscsit_allocate_iovecs(cmd) < 0) {
> 		return iscsit_reject_cmd(cmd,
> 				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
> 	}
> 	immed_data = cmd->immediate_data;
> 
> 	rc = iscsit_process_scsi_cmd(conn, cmd, hdr);
> 	if (rc < 0)
> 		return rc;
> 	else if (rc > 0)
> 		dump_payload = true;
> 
> 	if (!immed_data)
> 		return 0;
> 
> 	return iscsit_get_immediate_data(cmd, hdr, dump_payload);
> }
> 
> In other words, if target_setup_cmd_from_cdb() returns a sense code (positive
> value) iscsit_get_immediate_data() will call rx_data() to receive that immediate
> data if there is immediate data and dump_payload == false. The code that controls
> the value of dump_payload is as follows (from iscsit_process_scsi_cmd()):
> 
> 	if (cmd->sense_reason) {
> 		if (cmd->reject_reason)
> 			return 0;
> 
> 		return 1;
> 	}
> 
> In other words, if both .sense_reason and .reject_reason are set before
> iscsit_process_scsi_cmd() is called then iscsit_get_immediate_data() can call
> rx_data() to receive more data than what fits in the allocated buffer.
> 

Look again.  It's the code right below what you're referencing above, at
the bottom of iscsit_process_scsi_cmd():

        /*
         * Call directly into transport_generic_new_cmd() to perform
         * the backend memory allocation.
         */
        cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd);
        if (cmd->sense_reason)
                return 1;

which propigates '1' up to iscsit_handle_scsi_cmd(), passing
'dump_payload = true' into iscsit_get_immediate_data() and thereby calls
iscsit_dump_data_payload() to discard immediate data into a separate
throw away buffer.

Try it with sg_raw or libiscsi and you'll see.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Nicholas A. Bellinger]

On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2017-05-08 at 17:46 +0000, Bart Van Assche wrote:
> > On Sun, 2017-05-07 at 15:45 -0700, Nicholas A. Bellinger wrote:
> > > On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> > > > Avoid that the iSCSI target driver complains about "Initial page entry
> > > > out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> > > > is larger than the buffer size specified through the CDB. This patch
> > > > prevents that running the libiscsi regression tests against LIO trigger
> > > > an infinite loop of libiscsi submitting a command, LIO closing the
> > > > connection and libiscsi resubmitting the same command.
> > > > 
> > > 
> > > This statement and patch is incorrect.
> > > 
> > > target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with
> > > overflow/underflow since day one:
> > > 
> > > From target_cmd_size_check():
> > > 
> > >         } else if (size != cmd->data_length) {
> > >                 pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
> > >                         " %u does not match SCSI CDB Length: %u for SAM Opcode:"
> > >                         " 0x%02x\n", cmd->se_tfo->get_fabric_name(),
> > >                                 cmd->data_length, size, cmd->t_task_cdb[0]);
> > > 
> > >                 if (cmd->data_direction == DMA_TO_DEVICE &&
> > >                     cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
> > >                         pr_err("Rejecting underflow/overflow WRITE data\n");
> > >                         return TCM_INVALID_CDB_FIELD;
> > >                 }
> > > 
> > >                 ......
> > >         }
> > 
> > Hello Nic,
> > 
> > That behavior is as far as I know not compliant with the SCSI specs. In e.g.
> > the libiscsi test suite there are many tests that verify that a SCSI target
> > implementation reports OVERFLOW or UNDERFLOW where LIO today reports INVALID
> > FIELD IN CDB. See e.g. https://github.com/sahlberg/libiscsi/blob/master/test-tool/test_read10_residuals.c.
> 
> Yes, I understand what the test does.  In practice this has never been
> an issue, and we've not actually encountered a host that sends overflow
> or underflow for a WRITE CDB of type SCF_SCSI_DATA_CDB.
> 
> If there is a host environment that does, I'd like to hear about it.
> 
> In any event, the point is your patch to add sbc_parse_verify() broke
> existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB
> assignment for all cases.
> 
> > From RFC 3720 (apparently written from the point-of-view of transfers from
> > target to initiator):
> > 
> >      bit 5 - (O) set for Residual Overflow.  In this case, the Residual
> >        Count indicates the number of bytes that were not transferred
> >        because the initiator's Expected Data Transfer Length was not
> >        sufficient.
> > 
> >      bit 6 - (U) set for Residual Underflow.  In this case, the Residual
> >        Count indicates the number of bytes that were not transferred out
> >        of the number of bytes that were expected to be transferred.
> > 
> > But even with the current implementation, why do you think that the reject by
> > target_cmd_size_check() would be sufficient to prevent the behavior I described?
> > From source file iscsi_target.c:
> > 
> > static int
> > iscsit_handle_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> > 			   unsigned char *buf)
> > {
> > 	struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)buf;
> > 	int rc, immed_data;
> > 	bool dump_payload = false;
> > 
> > 	rc = iscsit_setup_scsi_cmd(conn, cmd, buf);
> > 	if (rc < 0)
> > 		return 0;
> > 	/*
> > 	 * Allocation iovecs needed for struct socket operations for
> > 	 * traditional iSCSI block I/O.
> > 	 */
> > 	if (iscsit_allocate_iovecs(cmd) < 0) {
> > 		return iscsit_reject_cmd(cmd,
> > 				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
> > 	}
> > 	immed_data = cmd->immediate_data;
> > 
> > 	rc = iscsit_process_scsi_cmd(conn, cmd, hdr);
> > 	if (rc < 0)
> > 		return rc;
> > 	else if (rc > 0)
> > 		dump_payload = true;
> > 
> > 	if (!immed_data)
> > 		return 0;
> > 
> > 	return iscsit_get_immediate_data(cmd, hdr, dump_payload);
> > }
> > 
> > In other words, if target_setup_cmd_from_cdb() returns a sense code (positive
> > value) iscsit_get_immediate_data() will call rx_data() to receive that immediate
> > data if there is immediate data and dump_payload == false. The code that controls
> > the value of dump_payload is as follows (from iscsit_process_scsi_cmd()):
> > 
> > 	if (cmd->sense_reason) {
> > 		if (cmd->reject_reason)
> > 			return 0;
> > 
> > 		return 1;
> > 	}
> > 
> > In other words, if both .sense_reason and .reject_reason are set before
> > iscsit_process_scsi_cmd() is called then iscsit_get_immediate_data() can call
> > rx_data() to receive more data than what fits in the allocated buffer.
> > 
> 
> Look again.  It's the code right below what you're referencing above, at
> the bottom of iscsit_process_scsi_cmd():
> 
>         /*
>          * Call directly into transport_generic_new_cmd() to perform
>          * the backend memory allocation.
>          */
>         cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd);
>         if (cmd->sense_reason)
>                 return 1;
> 
> which propigates '1' up to iscsit_handle_scsi_cmd(), passing
> 'dump_payload = true' into iscsit_get_immediate_data() and thereby calls
> iscsit_dump_data_payload() to discard immediate data into a separate
> throw away buffer.
> 
> Try it with sg_raw or libiscsi and you'll see.

Oh btw, to further clarify your earlier question about the following
code in iscsit_process_scsi_cmd() returning zero when cmd->sense_reason
&& cmd->reject_reason is true:

        if (cmd->sense_reason) {
                if (cmd->reject_reason)
                        return 0;

                return 1;
        }

The earlier callers that invoke iscsi_add_reject*() all return -1 from
iscsit_setup_scsi_cmd(), which ensures iscsit_handle_scsi_cmd() returns 
and this code to return '0' is never reached.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Bart Van Assche]

On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote:
> In any event, the point is your patch to add sbc_parse_verify() broke
> existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB
> assignment for all cases.

As I had already explained in detail I disagree with this statement. BTW, did
you know that your patch "target: Fix sbc_parse_verify bytchk = 0 handling" is
not sufficient to avoid a buffer overflow in the iSCSI target driver? One way
to trigger a buffer overflow is by making the initiator send more immediate
data than the Data-Out buffer size derived from the CDB.

Bart.

Re: [PATCH 05/19] target: Allocate sg-list correctly

[Nicholas A. Bellinger]

On Wed, 2017-05-10 at 20:31 +0000, Bart Van Assche wrote:
> On Tue, 2017-05-09 at 21:03 -0700, Nicholas A. Bellinger wrote:
> > In any event, the point is your patch to add sbc_parse_verify() broke
> > existing behavior of WRITE_VERIFY_* by dropping SCF_SCSI_DATA_CDB
> > assignment for all cases.
> 
> As I had already explained in detail I disagree with this statement. BTW, did
> you know that your patch "target: Fix sbc_parse_verify bytchk = 0 handling" is
> not sufficient to avoid a buffer overflow in the iSCSI target driver? One way
> to trigger a buffer overflow is by making the initiator send more immediate
> data than the Data-Out buffer size derived from the CDB.
> 

If you think you've found a legitimate bug, then post the test case to
trigger it atop what's in target-pending/for-next for -rc1.

Regardless, your WRITE_VERIFY changes broke existing behavior, and I'm
not going to merge a bunch of new code at the last minute.

[PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

For VERIFY and WRITE AND VERIFY commands the size of the SCSI
Data-Out buffer can differ from the size of the data area on
the storage medium that is affected by the command. Make sure
that the Data-Out buffer size is computed correctly. Apparently
this part got dropped from my previous VERIFY / WRITE AND VERIFY
patch before I posted it due to rebasing.

Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/target_core_sbc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a0ad618f1b1a..51489d96cb31 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -888,9 +888,10 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
+	enum { INVALID_SIZE = 1 };
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *cdb = cmd->t_task_cdb;
-	unsigned int size;
+	unsigned int size = INVALID_SIZE;
 	u32 sectors = 0;
 	sense_reason_t ret;
 
@@ -1212,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			return TCM_ADDRESS_OUT_OF_RANGE;
 		}
 
-		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
+		if (size == INVALID_SIZE)
 			size = sbc_get_size(cmd, sectors);
 	}
 
-- 
2.12.2

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Christoph Hellwig]

Hi Bart,

what do you think about the variant below instead which avoids
overloading the size variable?

---
>From 206696ec37cf4f6efe093964c2bdc96100de1f62 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 5 May 2017 11:40:38 +0200
Subject: target: fix and cleanup size calculation in sbc_parse_cdb

Calculate the data buffer size individually for each command instead of
trying to generalize it.  This fixes the size calculation for VERIFY
and WRITE_VERIFY, while making the code a lot easier to understand.

Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_sbc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5807123214e5..0bd879b9ce38 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -899,12 +899,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 	switch (cdb[0]) {
 	case READ_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -919,6 +921,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -933,6 +936,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -947,12 +951,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -974,6 +980,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		goto check_lba;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -988,6 +995,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -1005,6 +1013,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		    !(cmd->se_cmd_flags & SCF_BIDI))
 			return TCM_INVALID_CDB_FIELD;
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
 			return TCM_INVALID_CDB_FIELD;
@@ -1024,6 +1033,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		switch (service_action) {
 		case XDWRITEREAD_32:
 			sectors = transport_get_sectors_32(cdb);
+			size = sbc_get_size(cmd, sectors);
 
 			if (sbc_check_dpofua(dev, cmd, cdb))
 				return TCM_INVALID_CDB_FIELD;
@@ -1116,7 +1126,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			sectors = transport_get_sectors_16(cdb);
 			cmd->t_task_lba = transport_lba_64(cdb);
 		}
+	
+		/*
+		 * XXX: why treat sectors / size check differently for
+		 * the offload  / non-offload cases?
+		 */
 		if (ops->execute_sync_cache) {
+			size = sbc_get_size(cmd, sectors);
 			cmd->execute_cmd = ops->execute_sync_cache;
 			goto check_lba;
 		}
@@ -1205,9 +1221,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 				end_lba, cmd->t_task_lba, sectors);
 			return TCM_ADDRESS_OUT_OF_RANGE;
 		}
-
-		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
-			size = sbc_get_size(cmd, sectors);
 	}
 
 	return target_cmd_size_check(cmd, size);
-- 
2.11.0

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Fri, 2017-05-05 at 11:42 +0200, Christoph Hellwig wrote:
> what do you think about the variant below instead which avoids
> overloading the size variable?

Hello Christoph,

That should also work but results in slightly more code. I don't have a strong
opinion about which approach to select.

Bart.

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> Data-Out buffer can differ from the size of the data area on
> the storage medium that is affected by the command. Make sure
> that the Data-Out buffer size is computed correctly. Apparently
> this part got dropped from my previous VERIFY / WRITE AND VERIFY
> patch before I posted it due to rebasing.
> 
> Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_sbc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index a0ad618f1b1a..51489d96cb31 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -888,9 +888,10 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
>  sense_reason_t
>  sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>  {
> +	enum { INVALID_SIZE = 1 };
>  	struct se_device *dev = cmd->se_dev;
>  	unsigned char *cdb = cmd->t_task_cdb;
> -	unsigned int size;
> +	unsigned int size = INVALID_SIZE;
>  	u32 sectors = 0;
>  	sense_reason_t ret;
>  
> @@ -1212,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
>  			return TCM_ADDRESS_OUT_OF_RANGE;
>  		}
>  
> -		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
> +		if (size == INVALID_SIZE)
>  			size = sbc_get_size(cmd, sectors);
>  	}
>  

This patch has no effect.

All it does for VERIFY + WRITE_AND_VERIFY is prevent sbc_get_size() from
being called twice with the same parameters, because your previous patch
in for-next was incorrectly only doing it for only bytchk = 1.

Anyways, sbc_parse_verify() should not being doing this directly, and
not for only bytchk = 1.

Anyways, I've fixed both cases and will post the proper fix inline
against patch #19.

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Sun, 2017-05-07 at 15:49 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > Data-Out buffer can differ from the size of the data area on
> > the storage medium that is affected by the command. Make sure
> > that the Data-Out buffer size is computed correctly. Apparently
> > this part got dropped from my previous VERIFY / WRITE AND VERIFY
> > patch before I posted it due to rebasing.
> > 
> > Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Andy Grover <agrover@redhat.com>
> > Cc: David Disseldorp <ddiss@suse.de>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/target/target_core_sbc.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index a0ad618f1b1a..51489d96cb31 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -888,9 +888,10 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
> >  sense_reason_t
> >  sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> >  {
> > +	enum { INVALID_SIZE = 1 };
> >  	struct se_device *dev = cmd->se_dev;
> >  	unsigned char *cdb = cmd->t_task_cdb;
> > -	unsigned int size;
> > +	unsigned int size = INVALID_SIZE;
> >  	u32 sectors = 0;
> >  	sense_reason_t ret;
> >  
> > @@ -1212,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> >  			return TCM_ADDRESS_OUT_OF_RANGE;
> >  		}
> >  
> > -		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
> > +		if (size == INVALID_SIZE)
> >  			size = sbc_get_size(cmd, sectors);
> >  	}
> >  
> 
> This patch has no effect.

Hello Nic,

That's a misinterpretation of your side. What this patch does is to ensure
that 'size' remains zero if a VERIFY or WRITE AND VERIFY command is submitted
with BYTCHK == 0. SBC mentions clearly that BYTCHK == 0 means that there is
no Data-Out buffer, or in other words, that the size of the Data-Out buffer
is zero.

>From the sg_verify man page:

       When --ndo=NDO is not given then the verify starts at the logical block
       address given by the --lba=LBA option and continues for --count=COUNT
       blocks.

In other words, sg_verify is able to submit a VERIFY command without Data-Out
buffer (NDO is the size of the Data-Out buffer in bytes).

> Anyways, I've fixed both cases and will post the proper fix inline
> against patch #19.

Do you perhaps mean patch "target: Fix sbc_parse_verify bytchk = 0 handling"?
(https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?h=for-next&id=d7d40280f868463d01a82186fd61cdc0e590381f)

If you want to know my opinion about that patch: it doesn't fix anything but
breaks the sbc_parse_verify() function and definitely doesn't make the
behavior of VERIFY nor WRITE AND VERIFY more compliant with the SCSI specs.

Bart.

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Mon, 2017-05-08 at 18:07 +0000, Bart Van Assche wrote:
> On Sun, 2017-05-07 at 15:49 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > > Data-Out buffer can differ from the size of the data area on
> > > the storage medium that is affected by the command. Make sure
> > > that the Data-Out buffer size is computed correctly. Apparently
> > > this part got dropped from my previous VERIFY / WRITE AND VERIFY
> > > patch before I posted it due to rebasing.
> > > 
> > > Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > Cc: Hannes Reinecke <hare@suse.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Andy Grover <agrover@redhat.com>
> > > Cc: David Disseldorp <ddiss@suse.de>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > >  drivers/target/target_core_sbc.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > > index a0ad618f1b1a..51489d96cb31 100644
> > > --- a/drivers/target/target_core_sbc.c
> > > +++ b/drivers/target/target_core_sbc.c
> > > @@ -888,9 +888,10 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
> > >  sense_reason_t
> > >  sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> > >  {
> > > +	enum { INVALID_SIZE = 1 };
> > >  	struct se_device *dev = cmd->se_dev;
> > >  	unsigned char *cdb = cmd->t_task_cdb;
> > > -	unsigned int size;
> > > +	unsigned int size = INVALID_SIZE;
> > >  	u32 sectors = 0;
> > >  	sense_reason_t ret;
> > >  
> > > @@ -1212,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> > >  			return TCM_ADDRESS_OUT_OF_RANGE;
> > >  		}
> > >  
> > > -		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
> > > +		if (size == INVALID_SIZE)
> > >  			size = sbc_get_size(cmd, sectors);
> > >  	}
> > >  
> > 
> > This patch has no effect.
> 
> Hello Nic,
> 
> That's a misinterpretation of your side. What this patch does is to ensure
> that 'size' remains zero if a VERIFY or WRITE AND VERIFY command is submitted
> with BYTCHK == 0. SBC mentions clearly that BYTCHK == 0 means that there is
> no Data-Out buffer, or in other words, that the size of the Data-Out buffer
> is zero.
> 
> From the sg_verify man page:
> 
>        When --ndo=NDO is not given then the verify starts at the logical block
>        address given by the --lba=LBA option and continues for --count=COUNT
>        blocks.
> 
> In other words, sg_verify is able to submit a VERIFY command without Data-Out
> buffer (NDO is the size of the Data-Out buffer in bytes).
> 

That's fine the spec says no data is associated with bytchk = 0.

The point is the 'size' value in sbc_parse_cdb() is what's extracted
from the received CDB, and compared against extended data transfer
length (cmd->data_length) in target_cmd_size_check() to determine
overflow or underflow.

Attempting to make 'size' in sbc_parse_cdb() enforce what the spec says,
instead of what it actually is in the received CDB is completely wrong.
No other CDB does this, and *VERIFY* is no exception.

If you're worried about non zero transfer length with bytchk = 0 getting
processed, go ahead and return TCM_INVALID_CDB_FIELD from
sbc_parse_verify() when this happens, like every other sanity check
does.

Of course, that's exactly the test case libiscsi does during
test_writeverify16_residuals, send bythck = 0 with overflow and
underflow with a non zero transfer length.

> > Anyways, I've fixed both cases and will post the proper fix inline
> > against patch #19.
> 
> Do you perhaps mean patch "target: Fix sbc_parse_verify bytchk = 0 handling"?
> (https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?h=for-next&id=d7d40280f868463d01a82186fd61cdc0e590381f)
> 
> If you want to know my opinion about that patch: it doesn't fix anything but
> breaks the sbc_parse_verify() function and definitely doesn't make the
> behavior of VERIFY nor WRITE AND VERIFY more compliant with the SCSI specs.

You are incorrect.

It addresses the regression your code introduced that broke existing
behavior for WRITE_VERIFY_*, when sbc_parse_verify() dropped
SCF_SCSI_DATA_CDB for bytchk = 0 that allowed overflow to trigger an
NULL pointer dereference.

Perhaps you should verify existing code before your sbc_parse_verify()
changes to see what I'm talking about.

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Tue, 2017-05-09 at 21:28 -0700, Nicholas A. Bellinger wrote:
> Attempting to make 'size' in sbc_parse_cdb() enforce what the spec says,
> instead of what it actually is in the received CDB is completely wrong.

Please look again at e.g. the sg_verify source code. If it sets BYTCHK to
zero it doesn't submit a Data-Out buffer. The only initiator that submits
a Data-Out buffer and sets BYTCHK to zero is libiscsi when testing overflow
behavior.

Bart.

Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Wed, 2017-05-10 at 15:16 +0000, Bart Van Assche wrote:
> On Tue, 2017-05-09 at 21:28 -0700, Nicholas A. Bellinger wrote:
> > Attempting to make 'size' in sbc_parse_cdb() enforce what the spec says,
> > instead of what it actually is in the received CDB is completely wrong.
> 
> Please look again at e.g. the sg_verify source code. If it sets BYTCHK to
> zero it doesn't submit a Data-Out buffer. The only initiator that submits
> a Data-Out buffer and sets BYTCHK to zero is libiscsi when testing overflow
> behavior.

To repeat, it doesn't matter what the spec says, or what some host sends
or doesn't send.

The usage of 'size' in sbc_parse_cdb() is strictly for the value of the
transfer length extracted from a received CDB, and used to determine
overflow or underflow vs. fabric EDTL within target_cmd_size_check().

Any patch that attempts to arbitrarily set this to a value other than
what was received by a CDB that contains a transfer length field is
wrong.

As mentioned earlier, if you're genuinely concerned about btychk = 0
with a non zero transfer length, then I'm happy to apply the following
patch post -rc1:

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 2cc8753..af618a6 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -869,6 +869,9 @@ static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, unsigned int *sectors
 
        switch (bytchk) {
        case 0:
+               if (*sectors)
+                       return TCM_INVALID_CDB_FIELD;
+               /* fall-through */
        case 1:
                cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
                break;

[PATCH 17/19] target/iscsi: Simplify timer manipulation code

[Bart Van Assche]

Move timer initialization from before add_timer() to the context
where the containing object is initialized. Use setup_timer() and
mod_timer() instead of open coding these. Use 'jiffies' instead
of get_jiffies_64() when calculating expiry times because expiry
times have type unsigned long, just like 'jiffies'.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/iscsi/iscsi_target.c       |  3 +++
 drivers/target/iscsi/iscsi_target_erl0.c  | 10 +++-------
 drivers/target/iscsi/iscsi_target_erl0.h  |  1 +
 drivers/target/iscsi/iscsi_target_erl1.c  |  8 ++------
 drivers/target/iscsi/iscsi_target_erl1.h  |  1 +
 drivers/target/iscsi/iscsi_target_login.c | 16 ++++++++++------
 drivers/target/iscsi/iscsi_target_login.h |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  |  8 +++-----
 drivers/target/iscsi/iscsi_target_util.c  | 26 ++++++++------------------
 drivers/target/iscsi/iscsi_target_util.h  |  2 ++
 10 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 083e6228c99d..ef1bb12ee61e 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -372,6 +372,9 @@ struct iscsi_np *iscsit_add_np(
 	init_completion(&np->np_restart_comp);
 	INIT_LIST_HEAD(&np->np_list);
 
+	setup_timer(&np->np_login_timer, iscsi_handle_login_thread_timeout,
+		    (unsigned long)np);
+
 	ret = iscsi_target_setup_login_socket(np, sockaddr);
 	if (ret != 0) {
 		kfree(np);
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index 9a96e17bf7cd..0d7a2d9875c7 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -749,7 +749,7 @@ int iscsit_check_post_dataout(
 	}
 }
 
-static void iscsit_handle_time2retain_timeout(unsigned long data)
+void iscsit_handle_time2retain_timeout(unsigned long data)
 {
 	struct iscsi_session *sess = (struct iscsi_session *) data;
 	struct iscsi_portal_group *tpg = sess->tpg;
@@ -809,14 +809,10 @@ void iscsit_start_time2retain_handler(struct iscsi_session *sess)
 	pr_debug("Starting Time2Retain timer for %u seconds on"
 		" SID: %u\n", sess->sess_ops->DefaultTime2Retain, sess->sid);
 
-	init_timer(&sess->time2retain_timer);
-	sess->time2retain_timer.expires =
-		(get_jiffies_64() + sess->sess_ops->DefaultTime2Retain * HZ);
-	sess->time2retain_timer.data = (unsigned long)sess;
-	sess->time2retain_timer.function = iscsit_handle_time2retain_timeout;
 	sess->time2retain_timer_flags &= ~ISCSI_TF_STOP;
 	sess->time2retain_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&sess->time2retain_timer);
+	mod_timer(&sess->time2retain_timer,
+		  jiffies + sess->sess_ops->DefaultTime2Retain * HZ);
 }
 
 /*
diff --git a/drivers/target/iscsi/iscsi_target_erl0.h b/drivers/target/iscsi/iscsi_target_erl0.h
index 60e69e2af6ed..a54d2047ed1b 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.h
+++ b/drivers/target/iscsi/iscsi_target_erl0.h
@@ -11,6 +11,7 @@ extern void iscsit_set_dataout_sequence_values(struct iscsi_cmd *);
 extern int iscsit_check_pre_dataout(struct iscsi_cmd *, unsigned char *);
 extern int iscsit_check_post_dataout(struct iscsi_cmd *, unsigned char *, u8);
 extern void iscsit_start_time2retain_handler(struct iscsi_session *);
+extern void iscsit_handle_time2retain_timeout(unsigned long data);
 extern int iscsit_stop_time2retain_timer(struct iscsi_session *);
 extern void iscsit_connection_reinstatement_rcfr(struct iscsi_conn *);
 extern void iscsit_cause_connection_reinstatement(struct iscsi_conn *, int);
diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index 454f493174db..c774d93514f8 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -1166,7 +1166,7 @@ static int iscsit_set_dataout_timeout_values(
 /*
  *	NOTE: Called from interrupt (timer) context.
  */
-static void iscsit_handle_dataout_timeout(unsigned long data)
+void iscsit_handle_dataout_timeout(unsigned long data)
 {
 	u32 pdu_length = 0, pdu_offset = 0;
 	u32 r2t_length = 0, r2t_offset = 0;
@@ -1282,13 +1282,9 @@ void iscsit_start_dataout_timer(
 	pr_debug("Starting DataOUT timer for ITT: 0x%08x on"
 		" CID: %hu.\n", cmd->init_task_tag, conn->cid);
 
-	init_timer(&cmd->dataout_timer);
-	cmd->dataout_timer.expires = (get_jiffies_64() + na->dataout_timeout * HZ);
-	cmd->dataout_timer.data = (unsigned long)cmd;
-	cmd->dataout_timer.function = iscsit_handle_dataout_timeout;
 	cmd->dataout_timer_flags &= ~ISCSI_TF_STOP;
 	cmd->dataout_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&cmd->dataout_timer);
+	mod_timer(&cmd->dataout_timer, jiffies + na->dataout_timeout * HZ);
 }
 
 void iscsit_stop_dataout_timer(struct iscsi_cmd *cmd)
diff --git a/drivers/target/iscsi/iscsi_target_erl1.h b/drivers/target/iscsi/iscsi_target_erl1.h
index 54d36bd25bea..0ff6e310ca36 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.h
+++ b/drivers/target/iscsi/iscsi_target_erl1.h
@@ -29,6 +29,7 @@ extern int iscsit_execute_ooo_cmdsns(struct iscsi_session *);
 extern int iscsit_execute_cmd(struct iscsi_cmd *, int);
 extern int iscsit_handle_ooo_cmdsn(struct iscsi_session *, struct iscsi_cmd *, u32);
 extern void iscsit_remove_ooo_cmdsn(struct iscsi_session *, struct iscsi_ooo_cmdsn *);
+extern void iscsit_handle_dataout_timeout(unsigned long data);
 extern void iscsit_mod_dataout_timer(struct iscsi_cmd *);
 extern void iscsit_start_dataout_timer(struct iscsi_cmd *, struct iscsi_conn *);
 extern void iscsit_stop_dataout_timer(struct iscsi_cmd *);
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 66238477137b..cc2906b5866f 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -325,6 +325,9 @@ static int iscsi_login_zero_tsih_s1(
 	spin_lock_init(&sess->session_usage_lock);
 	spin_lock_init(&sess->ttt_lock);
 
+	setup_timer(&sess->time2retain_timer, iscsit_handle_time2retain_timeout,
+		    (unsigned long)sess);
+
 	idr_preload(GFP_KERNEL);
 	spin_lock_bh(&sess_idr_lock);
 	ret = idr_alloc(&sess_idr, NULL, 0, 0, GFP_NOWAIT);
@@ -833,7 +836,7 @@ void iscsi_post_login_handler(
 	iscsit_dec_conn_usage_count(conn);
 }
 
-static void iscsi_handle_login_thread_timeout(unsigned long data)
+void iscsi_handle_login_thread_timeout(unsigned long data)
 {
 	struct iscsi_np *np = (struct iscsi_np *) data;
 
@@ -860,13 +863,9 @@ static void iscsi_start_login_thread_timer(struct iscsi_np *np)
 	 * point we do not have access to ISCSI_TPG_ATTRIB(tpg)->login_timeout
 	 */
 	spin_lock_bh(&np->np_thread_lock);
-	init_timer(&np->np_login_timer);
-	np->np_login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ);
-	np->np_login_timer.data = (unsigned long)np;
-	np->np_login_timer.function = iscsi_handle_login_thread_timeout;
 	np->np_login_timer_flags &= ~ISCSI_TF_STOP;
 	np->np_login_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&np->np_login_timer);
+	mod_timer(&np->np_login_timer, jiffies + TA_LOGIN_TIMEOUT * HZ);
 
 	pr_debug("Added timeout timer to iSCSI login request for"
 			" %u seconds.\n", TA_LOGIN_TIMEOUT);
@@ -1258,6 +1257,11 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 	pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
 	conn->conn_state = TARG_CONN_STATE_FREE;
 
+	setup_timer(&conn->nopin_response_timer,
+		    iscsit_handle_nopin_response_timeout, (unsigned long)conn);
+	setup_timer(&conn->nopin_timer, iscsit_handle_nopin_timeout,
+		    (unsigned long)conn);
+
 	if (iscsit_conn_set_transport(conn, np->np_transport) < 0) {
 		kfree(conn);
 		return 1;
diff --git a/drivers/target/iscsi/iscsi_target_login.h b/drivers/target/iscsi/iscsi_target_login.h
index 0e1fd6cedd54..f77850bf3e58 100644
--- a/drivers/target/iscsi/iscsi_target_login.h
+++ b/drivers/target/iscsi/iscsi_target_login.h
@@ -24,5 +24,6 @@ extern void iscsi_post_login_handler(struct iscsi_np *, struct iscsi_conn *, u8)
 extern void iscsi_target_login_sess_out(struct iscsi_conn *, struct iscsi_np *,
 				bool, bool);
 extern int iscsi_target_login_thread(void *);
+extern void iscsi_handle_login_thread_timeout(unsigned long data);
 
 #endif   /*** ISCSI_TARGET_LOGIN_H ***/
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 7ccc9c1cbfd1..215399af0461 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -572,11 +572,9 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
 	conn->login_kworker = current;
 	allow_signal(SIGINT);
 
-	init_timer(&login_timer);
-	login_timer.expires = (get_jiffies_64() + TA_LOGIN_TIMEOUT * HZ);
-	login_timer.data = (unsigned long)conn;
-	login_timer.function = iscsi_target_login_timeout;
-	add_timer(&login_timer);
+	setup_timer_on_stack(&login_timer, iscsi_target_login_timeout,
+			     (unsigned long)conn);
+	mod_timer(&login_timer, jiffies + TA_LOGIN_TIMEOUT * HZ);
 	pr_debug("Starting login_timer for %s/%d\n", current->comm, current->pid);
 
 	rc = conn->conn_transport->iscsit_get_login_rx(conn, login);
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 1e36f83b5961..505dad5dab7f 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -176,6 +176,8 @@ struct iscsi_cmd *iscsit_allocate_cmd(struct iscsi_conn *conn, int state)
 	spin_lock_init(&cmd->istate_lock);
 	spin_lock_init(&cmd->error_lock);
 	spin_lock_init(&cmd->r2t_lock);
+	setup_timer(&cmd->dataout_timer, iscsit_handle_dataout_timeout,
+		    (unsigned long)cmd);
 
 	return cmd;
 }
@@ -880,7 +882,7 @@ static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response)
 	return 0;
 }
 
-static void iscsit_handle_nopin_response_timeout(unsigned long data)
+void iscsit_handle_nopin_response_timeout(unsigned long data)
 {
 	struct iscsi_conn *conn = (struct iscsi_conn *) data;
 
@@ -949,14 +951,10 @@ void iscsit_start_nopin_response_timer(struct iscsi_conn *conn)
 		return;
 	}
 
-	init_timer(&conn->nopin_response_timer);
-	conn->nopin_response_timer.expires =
-		(get_jiffies_64() + na->nopin_response_timeout * HZ);
-	conn->nopin_response_timer.data = (unsigned long)conn;
-	conn->nopin_response_timer.function = iscsit_handle_nopin_response_timeout;
 	conn->nopin_response_timer_flags &= ~ISCSI_TF_STOP;
 	conn->nopin_response_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&conn->nopin_response_timer);
+	mod_timer(&conn->nopin_response_timer,
+		  jiffies + na->nopin_response_timeout * HZ);
 
 	pr_debug("Started NOPIN Response Timer on CID: %d to %u"
 		" seconds\n", conn->cid, na->nopin_response_timeout);
@@ -980,7 +978,7 @@ void iscsit_stop_nopin_response_timer(struct iscsi_conn *conn)
 	spin_unlock_bh(&conn->nopin_timer_lock);
 }
 
-static void iscsit_handle_nopin_timeout(unsigned long data)
+void iscsit_handle_nopin_timeout(unsigned long data)
 {
 	struct iscsi_conn *conn = (struct iscsi_conn *) data;
 
@@ -1015,13 +1013,9 @@ void __iscsit_start_nopin_timer(struct iscsi_conn *conn)
 	if (conn->nopin_timer_flags & ISCSI_TF_RUNNING)
 		return;
 
-	init_timer(&conn->nopin_timer);
-	conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ);
-	conn->nopin_timer.data = (unsigned long)conn;
-	conn->nopin_timer.function = iscsit_handle_nopin_timeout;
 	conn->nopin_timer_flags &= ~ISCSI_TF_STOP;
 	conn->nopin_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&conn->nopin_timer);
+	mod_timer(&conn->nopin_timer, jiffies + na->nopin_timeout * HZ);
 
 	pr_debug("Started NOPIN Timer on CID: %d at %u second"
 		" interval\n", conn->cid, na->nopin_timeout);
@@ -1043,13 +1037,9 @@ void iscsit_start_nopin_timer(struct iscsi_conn *conn)
 		return;
 	}
 
-	init_timer(&conn->nopin_timer);
-	conn->nopin_timer.expires = (get_jiffies_64() + na->nopin_timeout * HZ);
-	conn->nopin_timer.data = (unsigned long)conn;
-	conn->nopin_timer.function = iscsit_handle_nopin_timeout;
 	conn->nopin_timer_flags &= ~ISCSI_TF_STOP;
 	conn->nopin_timer_flags |= ISCSI_TF_RUNNING;
-	add_timer(&conn->nopin_timer);
+	mod_timer(&conn->nopin_timer, jiffies + na->nopin_timeout * HZ);
 
 	pr_debug("Started NOPIN Timer on CID: %d at %u second"
 			" interval\n", conn->cid, na->nopin_timeout);
diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
index 425160565d0c..78b3b9d20963 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -47,9 +47,11 @@ extern struct iscsi_conn *iscsit_get_conn_from_cid_rcfr(struct iscsi_session *,
 extern void iscsit_check_conn_usage_count(struct iscsi_conn *);
 extern void iscsit_dec_conn_usage_count(struct iscsi_conn *);
 extern void iscsit_inc_conn_usage_count(struct iscsi_conn *);
+extern void iscsit_handle_nopin_response_timeout(unsigned long data);
 extern void iscsit_mod_nopin_response_timer(struct iscsi_conn *);
 extern void iscsit_start_nopin_response_timer(struct iscsi_conn *);
 extern void iscsit_stop_nopin_response_timer(struct iscsi_conn *);
+extern void iscsit_handle_nopin_timeout(unsigned long data);
 extern void __iscsit_start_nopin_timer(struct iscsi_conn *);
 extern void iscsit_start_nopin_timer(struct iscsi_conn *);
 extern void iscsit_stop_nopin_timer(struct iscsi_conn *);
-- 
2.12.2

Re: [PATCH 17/19] target/iscsi: Simplify timer manipulation code

[Hannes Reinecke]

On 05/05/2017 12:51 AM, Bart Van Assche wrote:
> Move timer initialization from before add_timer() to the context
> where the containing object is initialized. Use setup_timer() and
> mod_timer() instead of open coding these. Use 'jiffies' instead
> of get_jiffies_64() when calculating expiry times because expiry
> times have type unsigned long, just like 'jiffies'.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/iscsi/iscsi_target.c       |  3 +++
>  drivers/target/iscsi/iscsi_target_erl0.c  | 10 +++-------
>  drivers/target/iscsi/iscsi_target_erl0.h  |  1 +
>  drivers/target/iscsi/iscsi_target_erl1.c  |  8 ++------
>  drivers/target/iscsi/iscsi_target_erl1.h  |  1 +
>  drivers/target/iscsi/iscsi_target_login.c | 16 ++++++++++------
>  drivers/target/iscsi/iscsi_target_login.h |  1 +
>  drivers/target/iscsi/iscsi_target_nego.c  |  8 +++-----
>  drivers/target/iscsi/iscsi_target_util.c  | 26 ++++++++------------------
>  drivers/target/iscsi/iscsi_target_util.h  |  2 ++
>  10 files changed, 34 insertions(+), 42 deletions(-)

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)