* [PATCH 03/19] target: Avoid that aborting a command sporadically hangs [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> @ 2017-05-04 22:50 ` Bart Van Assche 2017-05-05 6:12 ` Hannes Reinecke ` (2 more replies) 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche ` (3 subsequent siblings) 4 siblings, 3 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-04 22:50 UTC (permalink / raw) To: Nicholas Bellinger Cc: target-devel, Bart Van Assche, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche @ 2017-05-05 6:12 ` Hannes Reinecke 2017-05-05 8:53 ` Christoph Hellwig 2017-05-07 22:20 ` Nicholas A. Bellinger 2 siblings, 0 replies; 32+ messages in thread From: Hannes Reinecke @ 2017-05-05 6:12 UTC (permalink / raw) To: Bart Van Assche, Nicholas Bellinger Cc: target-devel, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche 2017-05-05 6:12 ` Hannes Reinecke @ 2017-05-05 8:53 ` Christoph Hellwig 2017-05-05 15:00 ` Bart Van Assche 2017-05-11 0:23 ` Bart Van Assche 2017-05-07 22:20 ` Nicholas A. Bellinger 2 siblings, 2 replies; 32+ messages in thread From: Christoph Hellwig @ 2017-05-05 8:53 UTC (permalink / raw) To: Bart Van Assche Cc: Nicholas Bellinger, target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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)? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-05 8:53 ` Christoph Hellwig @ 2017-05-05 15:00 ` Bart Van Assche 2017-05-11 0:23 ` Bart Van Assche 1 sibling, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-05 15:00 UTC (permalink / raw) To: hch; +Cc: ddiss, hare, target-devel, agrover, nab, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-05 8:53 ` Christoph Hellwig 2017-05-05 15:00 ` Bart Van Assche @ 2017-05-11 0:23 ` Bart Van Assche 1 sibling, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-11 0:23 UTC (permalink / raw) To: hch; +Cc: ddiss, hare, target-devel, agrover, nab, stable [-- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche 2017-05-05 6:12 ` Hannes Reinecke 2017-05-05 8:53 ` Christoph Hellwig @ 2017-05-07 22:20 ` Nicholas A. Bellinger 2017-05-08 21:25 ` Bart Van Assche 2 siblings, 1 reply; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-07 22:20 UTC (permalink / raw) To: Bart Van Assche Cc: target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-07 22:20 ` Nicholas A. Bellinger @ 2017-05-08 21:25 ` Bart Van Assche 2017-05-10 4:48 ` Nicholas A. Bellinger 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-08 21:25 UTC (permalink / raw) To: nab; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs 2017-05-08 21:25 ` Bart Van Assche @ 2017-05-10 4:48 ` Nicholas A. Bellinger 0 siblings, 0 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-10 4:48 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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..? ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche @ 2017-05-04 22:50 ` Bart Van Assche 2017-05-05 6:14 ` Hannes Reinecke ` (2 more replies) 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche ` (2 subsequent siblings) 4 siblings, 3 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-04 22:50 UTC (permalink / raw) To: Nicholas Bellinger Cc: target-devel, Bart Van Assche, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable >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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche @ 2017-05-05 6:14 ` Hannes Reinecke 2017-05-05 8:54 ` Christoph Hellwig 2017-05-07 22:28 ` Nicholas A. Bellinger 2 siblings, 0 replies; 32+ messages in thread From: Hannes Reinecke @ 2017-05-05 6:14 UTC (permalink / raw) To: Bart Van Assche, Nicholas Bellinger Cc: target-devel, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche 2017-05-05 6:14 ` Hannes Reinecke @ 2017-05-05 8:54 ` Christoph Hellwig 2017-05-07 22:28 ` Nicholas A. Bellinger 2 siblings, 0 replies; 32+ messages in thread From: Christoph Hellwig @ 2017-05-05 8:54 UTC (permalink / raw) To: Bart Van Assche Cc: Nicholas Bellinger, target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche 2017-05-05 6:14 ` Hannes Reinecke 2017-05-05 8:54 ` Christoph Hellwig @ 2017-05-07 22:28 ` Nicholas A. Bellinger 2 siblings, 0 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-07 22:28 UTC (permalink / raw) To: Bart Van Assche Cc: target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 05/19] target: Allocate sg-list correctly [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche @ 2017-05-04 22:50 ` Bart Van Assche 2017-05-05 6:15 ` Hannes Reinecke ` (2 more replies) 2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche 2017-05-04 22:51 ` [PATCH 17/19] target/iscsi: Simplify timer manipulation code Bart Van Assche 4 siblings, 3 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-04 22:50 UTC (permalink / raw) To: Nicholas Bellinger Cc: target-devel, Bart Van Assche, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche @ 2017-05-05 6:15 ` Hannes Reinecke 2017-05-05 9:06 ` Christoph Hellwig 2017-05-07 22:45 ` Nicholas A. Bellinger 2 siblings, 0 replies; 32+ messages in thread From: Hannes Reinecke @ 2017-05-05 6:15 UTC (permalink / raw) To: Bart Van Assche, Nicholas Bellinger Cc: target-devel, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche 2017-05-05 6:15 ` Hannes Reinecke @ 2017-05-05 9:06 ` Christoph Hellwig 2017-05-05 15:49 ` Bart Van Assche 2017-05-07 22:45 ` Nicholas A. Bellinger 2 siblings, 1 reply; 32+ messages in thread From: Christoph Hellwig @ 2017-05-05 9:06 UTC (permalink / raw) To: Bart Van Assche Cc: Nicholas Bellinger, target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-05 9:06 ` Christoph Hellwig @ 2017-05-05 15:49 ` Bart Van Assche 0 siblings, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-05 15:49 UTC (permalink / raw) To: hch; +Cc: ddiss, hare, target-devel, agrover, nab, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche 2017-05-05 6:15 ` Hannes Reinecke 2017-05-05 9:06 ` Christoph Hellwig @ 2017-05-07 22:45 ` Nicholas A. Bellinger 2017-05-08 17:46 ` Bart Van Assche 2 siblings, 1 reply; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-07 22:45 UTC (permalink / raw) To: Bart Van Assche Cc: target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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; ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-07 22:45 ` Nicholas A. Bellinger @ 2017-05-08 17:46 ` Bart Van Assche 2017-05-10 4:03 ` Nicholas A. Bellinger 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-08 17:46 UTC (permalink / raw) To: nab; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-08 17:46 ` Bart Van Assche @ 2017-05-10 4:03 ` Nicholas A. Bellinger 2017-05-10 6:12 ` Nicholas A. Bellinger 2017-05-10 20:31 ` Bart Van Assche 0 siblings, 2 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-10 4:03 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-10 4:03 ` Nicholas A. Bellinger @ 2017-05-10 6:12 ` Nicholas A. Bellinger 2017-05-10 20:31 ` Bart Van Assche 1 sibling, 0 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-10 6:12 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-10 4:03 ` Nicholas A. Bellinger 2017-05-10 6:12 ` Nicholas A. Bellinger @ 2017-05-10 20:31 ` Bart Van Assche 2017-05-11 5:28 ` Nicholas A. Bellinger 1 sibling, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-10 20:31 UTC (permalink / raw) To: nab; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 05/19] target: Allocate sg-list correctly 2017-05-10 20:31 ` Bart Van Assche @ 2017-05-11 5:28 ` Nicholas A. Bellinger 0 siblings, 0 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-11 5:28 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> ` (2 preceding siblings ...) 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche @ 2017-05-04 22:50 ` Bart Van Assche 2017-05-05 9:42 ` Christoph Hellwig 2017-05-07 22:49 ` Nicholas A. Bellinger 2017-05-04 22:51 ` [PATCH 17/19] target/iscsi: Simplify timer manipulation code Bart Van Assche 4 siblings, 2 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-04 22:50 UTC (permalink / raw) To: Nicholas Bellinger Cc: target-devel, Bart Van Assche, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche @ 2017-05-05 9:42 ` Christoph Hellwig 2017-05-05 15:51 ` Bart Van Assche 2017-05-07 22:49 ` Nicholas A. Bellinger 1 sibling, 1 reply; 32+ messages in thread From: Christoph Hellwig @ 2017-05-05 9:42 UTC (permalink / raw) To: Bart Van Assche Cc: Nicholas Bellinger, target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-05 9:42 ` Christoph Hellwig @ 2017-05-05 15:51 ` Bart Van Assche 0 siblings, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-05-05 15:51 UTC (permalink / raw) To: hch; +Cc: ddiss, hare, target-devel, agrover, nab, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche 2017-05-05 9:42 ` Christoph Hellwig @ 2017-05-07 22:49 ` Nicholas A. Bellinger 2017-05-08 18:07 ` Bart Van Assche 1 sibling, 1 reply; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-07 22:49 UTC (permalink / raw) To: Bart Van Assche Cc: target-devel, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-07 22:49 ` Nicholas A. Bellinger @ 2017-05-08 18:07 ` Bart Van Assche 2017-05-10 4:28 ` Nicholas A. Bellinger 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-08 18:07 UTC (permalink / raw) To: nab; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-08 18:07 ` Bart Van Assche @ 2017-05-10 4:28 ` Nicholas A. Bellinger 2017-05-10 15:16 ` Bart Van Assche 0 siblings, 1 reply; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-10 4:28 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-10 4:28 ` Nicholas A. Bellinger @ 2017-05-10 15:16 ` Bart Van Assche 2017-05-11 5:09 ` Nicholas A. Bellinger 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-10 15:16 UTC (permalink / raw) To: nab; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands 2017-05-10 15:16 ` Bart Van Assche @ 2017-05-11 5:09 ` Nicholas A. Bellinger 0 siblings, 0 replies; 32+ messages in thread From: Nicholas A. Bellinger @ 2017-05-11 5:09 UTC (permalink / raw) To: Bart Van Assche; +Cc: hch, ddiss, hare, target-devel, agrover, stable 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; ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 17/19] target/iscsi: Simplify timer manipulation code [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> ` (3 preceding siblings ...) 2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche @ 2017-05-04 22:51 ` Bart Van Assche 2017-05-05 11:24 ` Hannes Reinecke 4 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-05-04 22:51 UTC (permalink / raw) To: Nicholas Bellinger Cc: target-devel, Bart Van Assche, Hannes Reinecke, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 17/19] target/iscsi: Simplify timer manipulation code 2017-05-04 22:51 ` [PATCH 17/19] target/iscsi: Simplify timer manipulation code Bart Van Assche @ 2017-05-05 11:24 ` Hannes Reinecke 0 siblings, 0 replies; 32+ messages in thread From: Hannes Reinecke @ 2017-05-05 11:24 UTC (permalink / raw) To: Bart Van Assche, Nicholas Bellinger Cc: target-devel, Christoph Hellwig, Andy Grover, David Disseldorp, stable 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2017-05-11 5:28 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com> 2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche 2017-05-05 6:12 ` Hannes Reinecke 2017-05-05 8:53 ` Christoph Hellwig 2017-05-05 15:00 ` Bart Van Assche 2017-05-11 0:23 ` Bart Van Assche 2017-05-07 22:20 ` Nicholas A. Bellinger 2017-05-08 21:25 ` Bart Van Assche 2017-05-10 4:48 ` Nicholas A. Bellinger 2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche 2017-05-05 6:14 ` Hannes Reinecke 2017-05-05 8:54 ` Christoph Hellwig 2017-05-07 22:28 ` Nicholas A. Bellinger 2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche 2017-05-05 6:15 ` Hannes Reinecke 2017-05-05 9:06 ` Christoph Hellwig 2017-05-05 15:49 ` Bart Van Assche 2017-05-07 22:45 ` Nicholas A. Bellinger 2017-05-08 17:46 ` Bart Van Assche 2017-05-10 4:03 ` Nicholas A. Bellinger 2017-05-10 6:12 ` Nicholas A. Bellinger 2017-05-10 20:31 ` Bart Van Assche 2017-05-11 5:28 ` Nicholas A. Bellinger 2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche 2017-05-05 9:42 ` Christoph Hellwig 2017-05-05 15:51 ` Bart Van Assche 2017-05-07 22:49 ` Nicholas A. Bellinger 2017-05-08 18:07 ` Bart Van Assche 2017-05-10 4:28 ` Nicholas A. Bellinger 2017-05-10 15:16 ` Bart Van Assche 2017-05-11 5:09 ` Nicholas A. Bellinger 2017-05-04 22:51 ` [PATCH 17/19] target/iscsi: Simplify timer manipulation code Bart Van Assche 2017-05-05 11:24 ` Hannes Reinecke
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.