Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Nicholas A. Bellinger]

Hello MNC & Co,

On Wed, 2018-10-10 at 11:58 -0500, Mike Christie wrote:
> On 10/09/2018 10:23 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger 
> > 
> > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes 
> > no
> > signals will be pending for task_struct executing the normal session 
> > shutdown
> > and I/O quiesce code-path.
> > 



> > diff --git a/drivers/target/target_core_transport.c 
> > b/drivers/target/target_core_transport.c
> > index 86c0156..fc3093d2 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
> > if (se_sess) {
> > spin_lock_irqsave(_sess->sess_cmd_lock, flags);
> > list_del_init(_cmd->se_cmd_list);
> > -   if (list_empty(_sess->sess_cmd_list))
> > +   if (se_sess->sess_tearing_down && 
> > list_empty(_sess->sess_cmd_list))
> 
> I think there is another issue with 00d909a107 and ibmvscsi_tgt.
> 
> The problem is that ibmvscsi_tgt never called
> target_sess_cmd_list_set_waiting. It only called
> target_wait_for_sess_cmds. So before 00d909a107 there was a bug in that
> driver and target_wait_for_sess_cmds never did what was intended because
> sess_wait_list would always be empty.
> 
> With 00d909a107, we no longer need to call
> target_sess_cmd_list_set_waiting to wait for outstanding commands, so
> for ibmvscsi_tgt will now wait for commands like we wanted. However, the
> commit added a WARN_ON that is hit if target_sess_cmd_list_set_waiting
> is not called, so we could hit that.
> 
> So I think we need to add a target_sess_cmd_list_set_waiting call in
> ibmvscsi_tgt to go along with your patch chunk above and make sure we do
> not trigger the WARN_ON.
> 

Nice catch.  :)

With target_wait_for_sess_cmd() usage pre 00d909a107 doing a list-splice
in target_sess_cmd_list_set_waiting(), this particular usage in
ibmvscsi_tgt has always been list_empty(>sess_wait_list) = true
(eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list)
since commit 712db3eb in 4.9.y code.

That said, ibmvscsi_tgt usage is very similar to vhost/scsi in the
respect individual /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/
endpoints used by VMs do not remove their I_T nexus while the VM is
active.

So AFAICT, ibmvscsi_tgt doesn't strictly need target_sess_wait_for_cmd()
at all if this is true.

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Nicholas A. Bellinger]

Hey Peter & Co,

On Wed, 2018-10-10 at 10:43 +0200, Peter Zijlstra wrote:
> On Wed, Oct 10, 2018 at 03:23:10AM +, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger 
> > 
> > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes 
> > no
> > signals will be pending for task_struct executing the normal session 
> > shutdown
> > and I/O quiesce code-path.
> > 
> > For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> > part of session shutdown.  This has been the behaviour since day one.
> 
> Not knowing much context here; but does it make sense for those
> kthreads to handle signals, ever? Most kthreads should be fine with
> ignore_signals().
> 

iscsi-target + ib-isert uses SIGINT amongst dedicated rx/tx connection
kthreads to signal connection shutdown, requiring in-flight se_cmd I/O
descriptors to be quiesced before making forward progress to release
se_session.

By the point wait_event_lock_irq_timeout() is called in the example
here, one of the two rx/tx connection kthreads has been stopped, and the
other kthread is still processing shutdown.  So while historically the
pending SIGINTs where not cleared (or ignored) during shutdown at this
point, there is no reason why they could not be ignored for iscsi-target
+ ib-isert.

That said, pre commit 00d909a107 code always used wait_for_completion()
and ignored pending signals.  As-is target_wait_for_sess_cmds() is
called directly from fabric driver code and in one case also from
user-space via configfs_write_file(), so AFAICT it does need
TASK_UNINTERRUPTIBLE.

[GIT PULL] target updates for v4.16-rc1

[Nicholas A. Bellinger]

Hi Linus,

Here are the target-pending updates for v4.16-rc1.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next

The highlights include:

- Numerous target-core-user improvements related to queue full and timeout 
handling. (MNC)
- Prevent target-core-user corruption when invalid data page is requested. (MNC)
- Add target-core device action configfs attributes to allow user-space to
  trigger events separate from existing attributes exposed to end-users. (MNC)
- Fix iscsi-target NULL pointer dereference 4.6+ regression in CHAP error path.
  (David Disseldorp)
- Avoid target-core backend UNMAP callbacks if range is zero. (Andrei Vagin)
- Fix a iscsi-target 4.14+ regression related multiple PDU logins, that
  was exposed due to removal of TCP prequeue support. (Florian Westphal + MNC)

Also, there is a iser-target bug still being worked on for post -rc1
code to address a long standing issue resulting in persistent
ib_post_send() failures, for RNICs with small max_send_sge.  

Thank you,

--nab

Andrei Vagin (1):
  target: don't call an unmap callback if a range length is zero

David Disseldorp (1):
  target/iscsi: avoid NULL dereference in CHAP auth error path

Florian Westphal (1):
  iscsi-target: make sure to wake up sleeping login worker

Luis de Bethencourt (1):
  tcmu: Fix trailing semicolon

Markus Elfring (7):
  sbp-target: Delete an error message for a failed memory allocation in
three functions
  target: tcm_loop: Delete an error message for a failed memory
allocation in four functions
  target: tcm_loop: Improve a size determination in two functions
  target: tcm_loop: Combine substrings for 26 messages
  target: tcm_loop: Delete two unnecessary variable initialisations in
tcm_loop_issue_tmr()
  target: tcm_loop: Delete an unnecessary return statement in
tcm_loop_submission_work()
  target: tcm_loop: Use seq_puts() in tcm_loop_show_info()

Mike Christie (19):
  tcmu: merge common block release code
  tcmu: split unmap_thread_fn
  tcmu: fix unmap thread race
  tcmu: move expired command completion to unmap thread
  tcmu: remove commands_lock
  tcmu: release blocks for partially setup cmds
  tcmu: simplify scatter_data_area error handling
  tcmu: fix free block calculation
  tcmu: prep queue_cmd_ring to be used by unmap wq
  tcmu: simplify dbi thresh handling
  tcmu: don't block submitting context for block waits
  tcmu: make ring buffer timer configurable
  tcmu: allow max block and global max blocks to be settable
  tcmu: prevent corruption when invalid data page requested
  target: add SAM_STAT_BUSY sense reason
  target_core_user: add cmd id to broken ring message
  target core: add device action configfs files
  tcmu: allow userspace to reset ring
  tcmu: fix cmd user after free

Rasmus Villemoes (1):
  target-core: don't use "const char*" for a buffer that is written to

Varun Prakash (1):
  cxgbit: call neigh_event_send() to update MAC address

Wei Yongjun (1):
  tcmu: fix error return code in tcmu_configure_device()

Xiubo Li (1):
  tcmu: clean up the scatter helper

tangwenji (2):
  tcmu: fix page addr in tcmu_flush_dcache_range
  target: fix destroy device in target_configure_device

 drivers/target/iscsi/cxgbit/cxgbit_cm.c  |   3 +
 drivers/target/iscsi/iscsi_target_auth.c |   3 +-
 drivers/target/iscsi/iscsi_target_nego.c |   3 +
 drivers/target/loopback/tcm_loop.c   | 145 ++---
 drivers/target/sbp/sbp_target.c  |  13 +-
 drivers/target/target_core_configfs.c|   6 +
 drivers/target/target_core_device.c  |   4 +-
 drivers/target/target_core_fabric_lib.c  |   6 +-
 drivers/target/target_core_internal.h|   3 +-
 drivers/target/target_core_pr.c  |   4 +-
 drivers/target/target_core_sbc.c |   8 +-
 drivers/target/target_core_transport.c   |   3 +
 drivers/target/target_core_user.c| 983 ++-
 include/target/target_core_backend.h |   1 +
 include/target/target_core_base.h|   2 +
 15 files changed, 798 insertions(+), 389 deletions(-)

Re: [PATCH target-pending] iscsi-target: make sure to wake up sleeping login worker

[Nicholas A. Bellinger]

Hey Florian & Co,

On Fri, 2018-01-19 at 18:26 +0100, Florian Westphal wrote:
> Eric Dumazet  wrote:
> > On Fri, 2018-01-19 at 14:36 +0100, Florian Westphal wrote:
> > > diff --git a/drivers/target/iscsi/iscsi_target_nego.c 
> > > b/drivers/target/iscsi/iscsi_target_nego.c
> > > index b686e2ce9c0e..3723f8f419aa 100644
> > > --- a/drivers/target/iscsi/iscsi_target_nego.c
> > > +++ b/drivers/target/iscsi/iscsi_target_nego.c
> > > @@ -432,6 +432,9 @@ static void iscsi_target_sk_data_ready(struct sock 
> > > *sk)
> > >   if (test_and_set_bit(LOGIN_FLAGS_READ_ACTIVE, >login_flags)) {
> > >   write_unlock_bh(>sk_callback_lock);
> > >   pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1, conn: %p \n", 
> > > conn);
> > > + if (WARN_ON(iscsi_target_sk_data_ready == 
> > > conn->orig_data_ready))
> > > + return;
> > 
> > Is this WARN_ON() belonging to this fix ?
> > At least make it WARN_ON_ONCE() or pr_err_once()
> 
> Nicholas, I don't know this code at all so it would be good if you could
> give advice here (omit all together, WARN_ON_ONCE, ...).
> 

This is regular behavior during multi PDU login sequences, and should
not include a WARN_ON.

So with MNC's Tested-by in place, applying to target-pending/for-next
minus the WARN_ON, with a extra 4.14.y stable tag.

Thanks again for taking a look at this.

To your earlier point wrt net.ipv4.tcp_low_latency=1 on 4.13 code not
triggering pre-queue logic.  From groking the original patch to drop
prequeue I agree this should really be the case, but am still at a loss
how MNC is triggering on 4.14+ unless something else has changed to
uncover this iscsi-target bug.

Still curious to verify the root cause, but I haven't been able to
reproduce this in VMs on small scale, and haven't had cycles to
reproduce on HW yet.

That said, since the bug appears to be masked on <= 4.13.y +
tcp_low_latency=1, unless someone can reproduce this on earlier code
with tcp_low_latency=0, I'll leave off the older stable tag for now.

Re: [PATCH] tcmu: Fix trailing semicolon

[Nicholas A. Bellinger]

On Tue, 2018-01-16 at 10:25 -0600, Michael Christie wrote:
> On 01/16/2018 09:34 AM, Luis de Bethencourt wrote:
> > The trailing semicolon is an empty statement that does no operation.
> > It is completely stripped out by the compiler. Removing it since it doesn't 
> > do
> > anything.
> > 
> > Signed-off-by: Luis de Bethencourt 
> > ---
> > 
> > Hi,
> > 
> > After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches
> > suggested I fix it treewide [0].
> > 
> > Best regards 
> > Luis
> > 
> > 
> > [0] 
> > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html
> > [1] 
> > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html
> > 
> >  drivers/target/target_core_user.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 

Applied.  Thanks Luis + MNC.

Re: [PATCH] target-core: don't use "const char*" for a buffer that is written to

[Nicholas A. Bellinger]

Hi Rasmus,

Apologies for the delayed follow-up on this.

On Tue, 2017-11-21 at 01:12 +0100, Rasmus Villemoes wrote:
> From: Rasmus Villemoes 
> 
> iscsi_parse_pr_out_transport_id launders the const away via a call to
> strstr(), and then modifies the buffer (writing a nul byte) through
> the return value. It's cleaner to be honest and simply declare the
> parameter as "char*", fixing up the call chain, and allowing us to
> drop the cast in the return statement.
> 
> Amusingly, the two current callers found it necessary to cast a
> non-const pointer to a const.
> 
> Signed-off-by: Rasmus Villemoes 
> ---
>  drivers/target/target_core_fabric_lib.c | 6 +++---
>  drivers/target/target_core_internal.h   | 2 +-
>  drivers/target/target_core_pr.c | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 

Looks fine.  Applied.

Thank you.

Re: [PATCH] target: don't call an unmap callback if a range length is zero

[Nicholas A. Bellinger]

Hi Andrei,

Apologies for the delayed follow up.

On Wed, 2017-12-13 at 13:55 -0800, Andrei Vagin wrote:
> If a length of a range is zero, it means there is nothing to unmap
> and we can skip this range.
> 
> Here is one more reason, why we have to skip such ranges.  An unmap
> callback calls file_operations->fallocate(), but the man page for the
> fallocate syscall says that fallocate(fd, mode, offset, let) returns
> EINVAL, if len is zero. It means that file_operations->fallocate() isn't
> obligated to handle zero ranges too.
> 
> Signed-off-by: Andrei Vagin 
> ---
>  drivers/target/target_core_sbc.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Applied.

Thank you.

[GIT PULL] target updates for v4.15-rc1

[Nicholas A. Bellinger]

Hello Linus,

Here are the target-pending updates for v4.15-rc1.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next

This series is predominantly bug-fixes, with a few small improvements
that have been outstanding over the last release cycle.

As usual, the associated bug-fixes have CC' tags for stable.

Also, things have been particularly quiet wrt new developments the last
months, with most folks continuing to focus on stability atop 4.x stable
kernels for their respective production configurations.

Also at this point, the stable trees have been synced up with mainline.
This will continue to be a priority, as production users tend to run
exclusively atop stable kernels, a few releases behind mainline.

The highlights include:

 - target: Fix PR PREEMPT_AND_ABORT null pointer dereference regression
   in v4.11+ (tangwenji)
 - target/user: Fix OOPs during removing TCMU device (Xiubo Li + Zhang Zhuoyu)
 - target/user: Add netlink command reply supported option for each device
(Kenjiro Nakayama)
 - cxgbit: Abort the TCP connection in case of data out timeout (Varun Prakash)
 - target: Fix PR/ALUA file path truncation (David Disseldorp)
 - target/user: Fix double se_cmd completion during ->cmd_time_out (Mike 
Christie)
 - target: Fix QUEUE_FULL + SCSI task attribute handling in 4.1+ (Bryant Ly + 
nab)
 - target: Fix quiese during transport_write_pending_qf endless loop (nab)
 - target: Avoid early CMD_T_PRE_EXECUTE failures during ABORT_TASK in 3.14+
   (Don White + nab)

Thank you,

--nab

Bart Van Assche (9):
  target: Move a declaration of a global variable into a header file
  target: Suppress gcc 7 fallthrough warnings
  target: Inline transport_put_cmd()
  target/iscsi: Define OFFLOAD_BUF_SIZE once
  target/iscsi: Use min() in iscsit_dump_data_payload() instead of
open-coding it
  target/iscsi: Fix endianness in an error message
  target/iscsi: Modify iscsit_do_crypto_hash_buf() prototype
  target/iscsi: Fix a race condition in iscsit_add_reject_from_cmd()
  target/iscsi: Detect conn_cmd_list corruption early

Dan Carpenter (2):
  tcmu: Fix some memory corruption
  tcmu: Add a missing unlock on an error path

David Disseldorp (2):
  target: fix PR state file path truncation
  target: fix ALUA state file path truncation

Jiang Yi (1):
  target/file: Do not return error for UNMAP if length is zero

Kenjiro Nakayama (2):
  target/tcmu: Use macro to call container_of in tcmu_cmd_time_out_show
  target: Add netlink command reply supported option for each device

Markus Elfring (1):
  iSCSI-target: Use common error handling code in
iscsi_decode_text_input()

Mike Christie (2):
  target: return SAM_STAT_TASK_SET_FULL for TCM_OUT_OF_RESOURCES
  tcmu: fix double se_cmd completion

Nicholas Bellinger (6):
  target: Fix QUEUE_FULL + SCSI task attribute handling
  target: Fix caw_sem leak in transport_generic_request_failure
  target: Fix quiese during transport_write_pending_qf endless loop
  target: Avoid early CMD_T_PRE_EXECUTE failures during ABORT_TASK
  iscsi-target: Make TASK_REASSIGN use proper se_cmd->cmd_kref
  iscsi-target: Fix non-immediate TMR reference leak

Varun Prakash (1):
  cxgbit: Abort the TCP connection in case of data out timeout

Xiubo Li (1):
  tcmu: fix crash when removing the tcmu device

tangwenji (8):
  target: fix null pointer regression in core_tmr_drain_tmr_list
  target: fix buffer offset in core_scsi3_pri_read_full_status
  target: fix double unmap data sg in
core_scsi3_emulate_pro_register_and_move()
  target: add sense code INSUFFICIENT REGISTRATION RESOURCES
  target: fix match_token option in target_core_configfs.c
  target:fix condition return in core_pr_dump_initiator_port()
  iscsi-target: fix memory leak in lio_target_tiqn_addtpg()
  iscsi-target: fix memory leak in iscsit_release_discovery_tpg()

 drivers/target/iscsi/cxgbit/cxgbit.h |   2 +
 drivers/target/iscsi/cxgbit/cxgbit_cm.c  |  45 +
 drivers/target/iscsi/cxgbit/cxgbit_ddp.c |   8 +
 drivers/target/iscsi/cxgbit/cxgbit_main.c|   1 +
 drivers/target/iscsi/iscsi_target.c  |  80 -
 drivers/target/iscsi/iscsi_target_configfs.c |   3 +-
 drivers/target/iscsi/iscsi_target_erl1.c |   7 +-
 drivers/target/iscsi/iscsi_target_parameters.c   |  39 ++---
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c |   2 -
 drivers/target/iscsi/iscsi_target_tpg.c  |   7 +-
 drivers/target/iscsi/iscsi_target_util.c |   4 +
 drivers/target/target_core_alua.c|  51 +++---
 drivers/target/target_core_alua.h|   9 -
 drivers/target/target_core_configfs.c|  14 +-
 drivers/target/target_core_fabric_configfs.c |   2 -
 drivers/target/target_core_file.c|   4 +
 drivers/target/target_core_internal.h|   1 +
 drivers/target/target_core_pr.c  |  41 ++---
 drivers/

Re: [PATCH 1/2] tcmu: Fix some memory corruption

[Nicholas A. Bellinger]

On Wed, 2017-11-08 at 11:43 +0300, Dan Carpenter wrote:
> "udev->nl_reply_supported" is an int but on 64 bit arches we are writing
> 8 bytes of data to it so it corrupts four bytes beyond the end of the
> struct.
> 
> Fixes: b849b4567549 ("target: Add netlink command reply supported option for 
> each device")
> Signed-off-by: Dan Carpenter 
> 

Applied to target-pending/for-next.

Re: [PATCH 2/2] tcmu: Add a missing unlock on an error path

[Nicholas A. Bellinger]

On Wed, 2017-11-08 at 11:44 +0300, Dan Carpenter wrote:
> We added a new error path here but we forgot to drop the lock first
> before returning.
> 
> Fixes: 0d44374c1aae ("tcmu: fix double se_cmd completion")
> Signed-off-by: Dan Carpenter 

Applied to target-pending/for-next.

Thanks DanC !

Re: target-pending/for-next patches

[Nicholas A. Bellinger]

On Sun, 2017-11-05 at 08:05 -0800, James Bottomley wrote:
> On Sat, 2017-11-04 at 18:14 -0700, Nicholas A. Bellinger wrote:
> > Hi all,
> > 
> > Just a friendly email after catching up on patches this week, the
> > majority of those outstanding on the list have been merged into
> > target-pending/for-next.  Please see below.
> > 
> > For those who submitted patches, please have a look and let me know
> > if anything is else missing.  Note there are two exceptions that have
> > been left out for now that I'll be following up with separately.
> > 
> > Thus far it's all been either straight-forward bug-fixes, minor
> > cleanups, or small miscellaneous enhancements.  AFAICT, nothing looks
> > particularly concerning.
> 
> The concern would be you dumping a tree on the eve of a merge window,
> which you're presumably going to send to Linus in a week or so, when
> the last time you appeared was a fixes pull in 12 August, because it
> suggests this lot is just some randomly chosen selection to try to keep
> the tree alive.

No.  The tree contains outstanding bug-fixes and very minor
improvements.

Do you have an objection to a particular patch..?

>  I really wouldn't do it like this: I know Linus
> doesn't care too much for SCSI stuff and if you're lucky he may be too
> busy yelling at Jens to notice, but if not, you'll find yourself on the
> receiving end of his ire and that will damage the reputation of your
> tree a lot.

I'd rather target-pending/for-next be judged on patches, and not
preconceived fear of including bug-fixes that address end-user issues
close to a merge window.

> 
> If the work of running the target tree has got too much, get a patch
> wrangler who can help with the process stuff you're completely lacking,
> like reviews and testing and long incubation in linux-next for exposure
> to 0day.

The past release has been exceedingly slow in drivers/target/, with the
rate of incoming patches is down to ~2 per week.  Those queued are
specific to drivers/target/ and don't run afoul of linux-next and 0-day
builds.

Personally, I'm still focused on bug-fixing and ensuring patches make it
back to v4.x.y linux-stable.git.  However, there are still few people
working on bugs specific to host <-> fabric <-> target-core <-> backend
configurations, beyond individual components fixes.

> I'm sure we can find several volunteers.

Sure, a patch wrangler would be helpful.

Where things tend to run into trouble is when someone starts merging
subsystem changes, but aren't directly responsible for distro releases
or production users running linux-stable.git code.

That said, I'd prefer someone with 'skin in the game' and end-users they
support.

[PATCH 1/6] target: Fix QUEUE_FULL + SCSI task attribute handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a bug during QUEUE_FULL where transport_complete_qf()
calls transport_complete_task_attr() after it's already been invoked
by target_complete_ok_work() or transport_generic_request_failure()
during initial completion, preceeding QUEUE_FULL.

This will result in se_device->simple_cmds, se_device->dev_cur_ordered_id
and/or se_device->dev_ordered_sync being updated multiple times for
a single se_cmd.

To address this bug, clear SCF_TASK_ATTR_SET after the first call
to transport_complete_task_attr(), and avoid updating SCSI task
attribute related counters for any subsequent calls.

Also, when a se_cmd is deferred due to ordered tags and executed
via target_restart_delayed_cmds(), set CMD_T_SENT before execution
matching what target_execute_cmd() does.

Cc: Michael Cyr <mike...@linux.vnet.ibm.com>
Cc: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 473d652..c33d1e9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2011,6 +2011,8 @@ static void target_restart_delayed_cmds(struct se_device 
*dev)
list_del(>se_delayed_node);
spin_unlock(>delayed_cmd_lock);
 
+   cmd->transport_state |= CMD_T_SENT;
+
__target_execute_cmd(cmd, true);
 
if (cmd->sam_task_attr == TCM_ORDERED_TAG)
@@ -2046,6 +2048,8 @@ static void transport_complete_task_attr(struct se_cmd 
*cmd)
pr_debug("Incremented dev_cur_ordered_id: %u for ORDERED\n",
 dev->dev_cur_ordered_id);
}
+   cmd->se_cmd_flags &= ~SCF_TASK_ATTR_SET;
+
 restart:
target_restart_delayed_cmds(dev);
 }
-- 
1.9.1

[PATCH 0/6] target fixes for v4.15-rc1

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Hi all,

Here are the outstanding target bugfixes in queue for v4.15-rc1
code.

Patch #1 addresses a long standing bug wrt to QUEUE_FULL and
SCSI task attribute handling, that results in SCSI task related
counters getting updated multiple times during QUEUE_FULL.

It primarly effects hosts using ORDERED tasks, which depend upon
these counters to know when to delay incoming tasks.

Patch #2 is for a recent v4.11+ regression, which during ABORT
of COMPARE_AND_WRITE can result in se_device->cam_sem getting
leaked due to se_cmd->transport_complete_callback() being
skipped.

Patch #3 addresses a possible end-less loop during QUEUE_FULL +
TFO->write_pending() failure, allowing se_cmd quiese to properly
complete the outstanding descriptor when requested.

Patch #4 addresses a use-after-tree that was hit in the field,
involving pre-backend execution se_cmd exceptions + subsequent
ABORT_TASK for a matching tag.

Patch #5 + #6 address a iscsi-target TMR related memory and
se_cmd->cmd_kref reference leaks respectively.

We've been testing #4, #5, and #6 internally on v4.1.y stable
code, and have not run into additional regressions.

The rest are straight-forward.

Please review.

--nab

Nicholas Bellinger (6):
  target: Fix QUEUE_FULL + SCSI task attribute handling
  target: Fix caw_sem leak in transport_generic_request_failure
  target: Fix quiese during transport_write_pending_qf endless loop
  target: Avoid early CMD_T_PRE_EXECUTE failures during ABORT_TASK
  iscsi-target: Make TASK_REASSIGN use proper se_cmd->cmd_kref
  iscsi-target: Fix non-immediate TMR reference leak

 drivers/target/iscsi/iscsi_target.c| 30 ++
 drivers/target/target_core_tmr.c   |  9 +
 drivers/target/target_core_transport.c | 26 +++---
 include/target/target_core_base.h  |  1 +
 4 files changed, 47 insertions(+), 19 deletions(-)

-- 
1.9.1

[PATCH 4/6] target: Avoid early CMD_T_PRE_EXECUTE failures during ABORT_TASK

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes bug where early se_cmd exceptions that occur
before backend execution can result in use-after-free if/when
a subsequent ABORT_TASK occurs for the same tag.

Since an early se_cmd exception will have had se_cmd added to
se_session->sess_cmd_list via target_get_sess_cmd(), it will
not have CMD_T_COMPLETE set by the usual target_complete_cmd()
backend completion path.

This causes a subsequent ABORT_TASK + __target_check_io_state()
to signal ABORT_TASK should proceed.  As core_tmr_abort_task()
executes, it will bring the outstanding se_cmd->cmd_kref count
down to zero releasing se_cmd, after se_cmd has already been
queued with error status into fabric driver response path code.

To address this bug, introduce a CMD_T_PRE_EXECUTE bit that is
set at target_get_sess_cmd() time, and cleared immediately before
backend driver dispatch in target_execute_cmd() once CMD_T_ACTIVE
is set.

Then, check CMD_T_PRE_EXECUTE within __target_check_io_state() to
determine when an early exception has occured, and avoid aborting
this se_cmd since it will have already been queued into fabric
driver response path code.

Reported-by: Donald White <d...@datera.io>
Cc: Donald White <d...@datera.io>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_tmr.c   | 9 +
 drivers/target/target_core_transport.c | 2 ++
 include/target/target_core_base.h  | 1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 61909b2..9c7bc1c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -133,6 +133,15 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
spin_unlock(_cmd->t_state_lock);
return false;
}
+   if (se_cmd->transport_state & CMD_T_PRE_EXECUTE) {
+   if (se_cmd->scsi_status) {
+   pr_debug("Attempted to abort io tag: %llu early failure"
+" status: 0x%02x\n", se_cmd->tag,
+se_cmd->scsi_status);
+   spin_unlock(_cmd->t_state_lock);
+   return false;
+   }
+   }
if (sess->sess_tearing_down || se_cmd->cmd_wait_set) {
pr_debug("Attempted to abort io tag: %llu already shutdown,"
" skipping\n", se_cmd->tag);
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 0e89db8..58caacd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1975,6 +1975,7 @@ void target_execute_cmd(struct se_cmd *cmd)
}
 
cmd->t_state = TRANSPORT_PROCESSING;
+   cmd->transport_state &= ~CMD_T_PRE_EXECUTE;
cmd->transport_state |= CMD_T_ACTIVE | CMD_T_SENT;
spin_unlock_irq(>t_state_lock);
 
@@ -2667,6 +2668,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool 
ack_kref)
ret = -ESHUTDOWN;
goto out;
}
+   se_cmd->transport_state |= CMD_T_PRE_EXECUTE;
list_add_tail(_cmd->se_cmd_list, _sess->sess_cmd_list);
 out:
spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index d3139a9..ccf501b 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -490,6 +490,7 @@ struct se_cmd {
 #define CMD_T_STOP (1 << 5)
 #define CMD_T_TAS  (1 << 10)
 #define CMD_T_FABRIC_STOP  (1 << 11)
+#define CMD_T_PRE_EXECUTE  (1 << 12)
spinlock_t  t_state_lock;
struct kref cmd_kref;
struct completion   t_transport_stop_comp;
-- 
1.9.1

[PATCH 3/6] target: Fix quiese during transport_write_pending_qf endless loop

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a potential end-less loop during QUEUE_FULL,
where cmd->se_tfo->write_pending() callback fails repeatedly
but __transport_wait_for_tasks() has already been invoked to
quiese the outstanding se_cmd descriptor.

To address this bug, this patch adds a CMD_T_STOP|CMD_T_ABORTED
check within transport_write_pending_qf() and invokes the
existing se_cmd->t_transport_stop_comp to signal quiese
completion back to __transport_wait_for_tasks().

Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
Cc: Michael Cyr <mike...@linux.vnet.ibm.com>
Cc: Potnuri Bharat Teja <bha...@chelsio.com>
Cc: Sagi Grimberg <s...@grimberg.me>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index d02218c..0e89db8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2560,7 +2560,20 @@ void transport_kunmap_data_sg(struct se_cmd *cmd)
 
 static void transport_write_pending_qf(struct se_cmd *cmd)
 {
+   unsigned long flags;
int ret;
+   bool stop;
+
+   spin_lock_irqsave(>t_state_lock, flags);
+   stop = (cmd->transport_state & (CMD_T_STOP | CMD_T_ABORTED));
+   spin_unlock_irqrestore(>t_state_lock, flags);
+
+   if (stop) {
+   pr_debug("%s:%d CMD_T_STOP|CMD_T_ABORTED for ITT: 0x%08llx\n",
+   __func__, __LINE__, cmd->tag);
+   complete_all(>t_transport_stop_comp);
+   return;
+   }
 
ret = cmd->se_tfo->write_pending(cmd);
if (ret) {
-- 
1.9.1

[PATCH 2/6] target: Fix caw_sem leak in transport_generic_request_failure

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

With the recent addition of transport_check_aborted_status() within
transport_generic_request_failure() to avoid sending a SCSI status
exception after CMD_T_ABORTED w/ TAS=1 has occured, it introduced
a COMPARE_AND_WRITE early failure regression.

Namely when COMPARE_AND_WRITE fails and se_device->caw_sem has
been taken by sbc_compare_and_write(), if the new check for
transport_check_aborted_status() returns true and exits,
cmd->transport_complete_callback() -> compare_and_write_post()
is skipped never releasing se_device->caw_sem.

This regression was originally introduced by:

  commit e3b88ee95b4e4bf3e9729a4695d695b9c7c296c8
  Author: Bart Van Assche <bart.vanass...@sandisk.com>
  Date:   Tue Feb 14 16:25:45 2017 -0800

  target: Fix handling of aborted failed commands

To address this bug, move the transport_check_aborted_status()
call after transport_complete_task_attr() and
cmd->transport_complete_callback().

Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Bart Van Assche <bart.vanass...@sandisk.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index c33d1e9..d02218c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1729,9 +1729,6 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 {
int ret = 0, post_ret = 0;
 
-   if (transport_check_aborted_status(cmd, 1))
-   return;
-
pr_debug("-[ Storage Engine Exception; sense_reason %d\n",
 sense_reason);
target_show_cmd("-[ ", cmd);
@@ -1740,6 +1737,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 * For SAM Task Attribute emulation for failed struct se_cmd
 */
transport_complete_task_attr(cmd);
+
/*
 * Handle special case for COMPARE_AND_WRITE failure, where the
 * callback is expected to drop the per device ->caw_sem.
@@ -1748,6 +1746,9 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 cmd->transport_complete_callback)
cmd->transport_complete_callback(cmd, false, _ret);
 
+   if (transport_check_aborted_status(cmd, 1))
+   return;
+
switch (sense_reason) {
case TCM_NON_EXISTENT_LUN:
case TCM_UNSUPPORTED_SCSI_OPCODE:
-- 
1.9.1

[PATCH 5/6] iscsi-target: Make TASK_REASSIGN use proper se_cmd->cmd_kref

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Since commit 59b6986dbf fixed a potential NULL pointer dereference
by allocating a se_tmr_req for ISCSI_TM_FUNC_TASK_REASSIGN, the
se_tmr_req is currently leaked by iscsit_free_cmd() because no
iscsi_cmd->se_cmd.se_tfo was associated.

To address this, treat ISCSI_TM_FUNC_TASK_REASSIGN like any other
TMR and call transport_init_se_cmd() + target_get_sess_cmd() to
setup iscsi_cmd->se_cmd.se_tfo with se_cmd->cmd_kref of 2.

This will ensure normal release operation once se_cmd->cmd_kref
reaches zero and target_release_cmd_kref() is invoked, se_tmr_req
will be released via existing target_free_cmd_mem() and
core_tmr_release_req() code.

Reported-by: Donald White <d...@datera.io>
Cc: Donald White <d...@datera.io>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 541f66a..048d422 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1955,7 +1955,6 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 
iscsi_tmf)
struct iscsi_tmr_req *tmr_req;
struct iscsi_tm *hdr;
int out_of_order_cmdsn = 0, ret;
-   bool sess_ref = false;
u8 function, tcm_function = TMR_UNKNOWN;
 
hdr = (struct iscsi_tm *) buf;
@@ -1988,22 +1987,23 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 
iscsi_tmf)
 
cmd->data_direction = DMA_NONE;
cmd->tmr_req = kzalloc(sizeof(*cmd->tmr_req), GFP_KERNEL);
-   if (!cmd->tmr_req)
+   if (!cmd->tmr_req) {
return iscsit_add_reject_cmd(cmd,
 ISCSI_REASON_BOOKMARK_NO_RESOURCES,
 buf);
+   }
+
+   transport_init_se_cmd(>se_cmd, _ops,
+ conn->sess->se_sess, 0, DMA_NONE,
+ TCM_SIMPLE_TAG, cmd->sense_buffer + 2);
+
+   target_get_sess_cmd(>se_cmd, true);
 
/*
 * TASK_REASSIGN for ERL=2 / connection stays inside of
 * LIO-Target $FABRIC_MOD
 */
if (function != ISCSI_TM_FUNC_TASK_REASSIGN) {
-   transport_init_se_cmd(>se_cmd, _ops,
- conn->sess->se_sess, 0, DMA_NONE,
- TCM_SIMPLE_TAG, cmd->sense_buffer + 2);
-
-   target_get_sess_cmd(>se_cmd, true);
-   sess_ref = true;
tcm_function = iscsit_convert_tmf(function);
if (tcm_function == TMR_UNKNOWN) {
pr_err("Unknown iSCSI TMR Function:"
@@ -2119,12 +2119,8 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 
iscsi_tmf)
 * For connection recovery, this is also the default action for
 * TMR TASK_REASSIGN.
 */
-   if (sess_ref) {
-   pr_debug("Handle TMR, using sess_ref=true check\n");
-   target_put_sess_cmd(>se_cmd);
-   }
-
iscsit_add_cmd_to_response_queue(cmd, conn, cmd->i_state);
+   target_put_sess_cmd(>se_cmd);
return 0;
 }
 EXPORT_SYMBOL(iscsit_handle_task_mgt_cmd);
-- 
1.9.1

[PATCH 6/6] iscsi-target: Fix non-immediate TMR reference leak

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a se_cmd->cmd_kref reference leak that can
occur when a non immediate TMR is proceeded our of command
sequence number order, and CMDSN_LOWER_THAN_EXP is returned
by iscsit_sequence_cmd().

To address this bug, call target_put_sess_cmd() during this
special case following what iscsit_process_scsi_cmd() does
upon CMDSN_LOWER_THAN_EXP.

Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 048d422..3b7bb58 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -2094,12 +2094,14 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 
iscsi_tmf)
 
if (!(hdr->opcode & ISCSI_OP_IMMEDIATE)) {
int cmdsn_ret = iscsit_sequence_cmd(conn, cmd, buf, hdr->cmdsn);
-   if (cmdsn_ret == CMDSN_HIGHER_THAN_EXP)
+   if (cmdsn_ret == CMDSN_HIGHER_THAN_EXP) {
out_of_order_cmdsn = 1;
-   else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP)
+   } else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
+   target_put_sess_cmd(>se_cmd);
return 0;
-   else if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
+   } else if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) {
return -1;
+   }
}
iscsit_ack_from_expstatsn(conn, be32_to_cpu(hdr->exp_statsn));
 
-- 
1.9.1

target-pending/for-next patches

[Nicholas A. Bellinger]

Hi all,

Just a friendly email after catching up on patches this week, the
majority of those outstanding on the list have been merged into
target-pending/for-next.  Please see below.

For those who submitted patches, please have a look and let me know if
anything is else missing.  Note there are two exceptions that have been
left out for now that I'll be following up with separately.

Thus far it's all been either straight-forward bug-fixes, minor
cleanups, or small miscellaneous enhancements.  AFAICT, nothing looks
particularly concerning.

Thank you,

--nab

17c45b9 iSCSI-target: Use common error handling code in 
iscsi_decode_text_input()
6eaf69e target/iscsi: Detect conn_cmd_list corruption early
cfe2b62 target/iscsi: Fix a race condition in iscsit_add_reject_from_cmd()
e1dfb21 target/iscsi: Modify iscsit_do_crypto_hash_buf() prototype
de3493a target/iscsi: Fix endianness in an error message
919765e target/iscsi: Use min() in iscsit_dump_data_payload() instead of 
open-coding it
8d973ab target/iscsi: Define OFFLOAD_BUF_SIZE once
c017069 target: Inline transport_put_cmd()
d7e595d target: Suppress gcc 7 fallthrough warnings
c48e559 target: Move a declaration of a global variable into a header file
0d44374 tcmu: fix double se_cmd completion
a271eac target: return SAM_STAT_TASK_SET_FULL for TCM_OUT_OF_RESOURCES
55435ba target: fix ALUA state file path truncation
bdc79f0 target: fix PR state file path truncation
1ae0172 cxgbit: Abort the TCP connection in case of data out timeout
b849b45 target: Add netlink command reply supported option for each device
b5ab697 target/tcmu: Use macro to call container_of in tcmu_cmd_time_out_show
c22adc0 tcmu: fix crash when removing the tcmu device
a0884d4 iscsi-target: fix memory leak in iscsit_release_discovery_tpg()
12d5a43 iscsi-target: fix memory leak in lio_target_tiqn_addtpg()
24528f0 target:fix condition return in core_pr_dump_initiator_port()
a2db857 target: fix match_token option in target_core_configfs.c
79dd6f2 target: add sense code INSUFFICIENT REGISTRATION RESOURCES
e437fa3 target: fix double unmap data sg in 
core_scsi3_emulate_pro_register_and_move()
c58a252 target: fix buffer offset in core_scsi3_pri_read_full_status
88fb2fa target: fix null pointer regression in core_tmr_drain_tmr_list
594e25e target/file: Do not return error for UNMAP if length is zero

[GIT PULL] target fixes for v4.13-rc5

[Nicholas A. Bellinger]

Hi Linus,

Here are the target-pending fixes for v4.13-rc5.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git master

The highlights include:

- Fix iscsi-target payload memory leak during ISCSI_FLAG_TEXT_CONTINUE
  (Varun Prakash)
- Fix tcm_qla2xxx incorrect use of tcm_qla2xxx_free_cmd during ABORT
  (Pascal de Bruijn + Himanshu Madhani + nab)
- Fix iscsi-target long-standing issue with parallel delete of a single
  network portal across multiple target instances (Gary Guo + nab)
- Fix target dynamic se_node GPF during uncached shutdown regression
  (Justin Maggard + nab)

Thank you,

--nab

Bryant G. Ly (1):
  tcmu: free old string on reconfig

Nicholas Bellinger (3):
  qla2xxx: Fix incorrect tcm_qla2xxx_free_cmd use during TMR ABORT (v2)
  iscsi-target: Fix iscsi_np reset hung task during parallel delete
  target: Fix node_acl demo-mode + uncached dynamic shutdown regression

Varun Prakash (4):
  cxgbit: add missing __kfree_skb()
  iscsi-target: fix memory leak in iscsit_setup_text_cmd()
  iscsi-target: fix invalid flags in text response
  cxgbit: fix sg_nents calculation

Xiubo Li (1):
  tcmu: Fix possible to/from address overflow when doing the memcpy

 drivers/scsi/qla2xxx/tcm_qla2xxx.c  | 30 -
 drivers/target/iscsi/cxgbit/cxgbit_cm.c | 16 +++
 drivers/target/iscsi/cxgbit/cxgbit_target.c | 12 +++-
 drivers/target/iscsi/iscsi_target.c |  6 --
 drivers/target/iscsi/iscsi_target_login.c   |  7 +--
 drivers/target/target_core_tpg.c|  4 ++--
 drivers/target/target_core_transport.c  |  4 ++--
 drivers/target/target_core_user.c   | 13 +++--
 include/target/iscsi/iscsi_target_core.h|  1 +
 9 files changed, 40 insertions(+), 53 deletions(-)

Re: [PATCH] tcmu: Oops in unmap_thread_fn()

[Nicholas A. Bellinger]

On Tue, 2017-08-01 at 23:09 +0300, Dan Carpenter wrote:
> Calling list_del() on the iterator pointer in list_for_each_entry() will
> cause an oops.  We need to user the _safe() version for that.
> 
> Fixes: c73d02f63c16 ("tcmu: Add fifo type waiter list support to avoid 
> starvation")
> Signed-off-by: Dan Carpenter 
> 

Applied to target-pending/for-next.

Thanks DanC.

[PATCH] iscsi-target: Fix iscsi_np reset hung task during parallel delete

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a bug associated with iscsit_reset_np_thread()
that can occur during parallel configfs rmdir of a single iscsi_np
used across multiple iscsi-target instances, that would result in
hung task(s) similar to below where configfs rmdir process context
was blocked indefinately waiting for iscsi_np->np_restart_comp
to finish:

[ 6726.112076] INFO: task dcp_proxy_node_:15550 blocked for more than 120 
seconds.
[ 6726.119440]   Tainted: GW  O 4.1.26-3321 #2
[ 6726.125045] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this 
message.
[ 6726.132927] dcp_proxy_node_ D 8803f202bc88 0 15550  1 0x
[ 6726.140058]  8803f202bc88 88085c64d960 88083b3b1ad0 
88087fffeb08
[ 6726.147593]  8803f202c000 7fff 88083f459c28 
88083b3b1ad0
[ 6726.155132]  88035373c100 8803f202bca8 8168ced2 
8803f202bcb8
[ 6726.162667] Call Trace:
[ 6726.165150]  [] schedule+0x32/0x80
[ 6726.170156]  [] schedule_timeout+0x214/0x290
[ 6726.176030]  [] ? __send_signal+0x52/0x4a0
[ 6726.181728]  [] wait_for_completion+0x96/0x100
[ 6726.187774]  [] ? wake_up_state+0x10/0x10
[ 6726.193395]  [] iscsit_reset_np_thread+0x62/0xe0 
[iscsi_target_mod]
[ 6726.201278]  [] iscsit_tpg_disable_portal_group+0x96/0x190 
[iscsi_target_mod]
[ 6726.210033]  [] lio_target_tpg_store_enable+0x4f/0xc0 
[iscsi_target_mod]
[ 6726.218351]  [] configfs_write_file+0xaa/0x110
[ 6726.224392]  [] vfs_write+0xa4/0x1b0
[ 6726.229576]  [] SyS_write+0x41/0xb0
[ 6726.234659]  [] system_call_fastpath+0x12/0x71

It would happen because each iscsit_reset_np_thread() sets state
to ISCSI_NP_THREAD_RESET, sends SIGINT, and then blocks waiting
for completion on iscsi_np->np_restart_comp.

However, if iscsi_np was active processing a login request and
more than a single iscsit_reset_np_thread() caller to the same
iscsi_np was blocked on iscsi_np->np_restart_comp, iscsi_np
kthread process context in __iscsi_target_login_thread() would
flush pending signals and only perform a single completion of
np->np_restart_comp before going back to sleep within transport
specific iscsit_transport->iscsi_accept_np code.

To address this bug, add a iscsi_np->np_reset_count and update
__iscsi_target_login_thread() to keep completing np->np_restart_comp
until ->np_reset_count has reached zero.

Reported-by: Gary Guo <g...@datera.io>
Tested-by: Gary Guo <g...@datera.io>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.de>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c   | 1 +
 drivers/target/iscsi/iscsi_target_login.c | 7 +--
 include/target/iscsi/iscsi_target_core.h  | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 12803de..5001261 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -418,6 +418,7 @@ int iscsit_reset_np_thread(
return 0;
}
np->np_thread_state = ISCSI_NP_THREAD_RESET;
+   atomic_inc(>np_reset_count);
 
if (np->np_thread) {
spin_unlock_bh(>np_thread_lock);
diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index e9bdc8b..dc13afb 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1243,9 +1243,11 @@ static int __iscsi_target_login_thread(struct iscsi_np 
*np)
flush_signals(current);
 
spin_lock_bh(>np_thread_lock);
-   if (np->np_thread_state == ISCSI_NP_THREAD_RESET) {
+   if (atomic_dec_if_positive(>np_reset_count) >= 0) {
np->np_thread_state = ISCSI_NP_THREAD_ACTIVE;
+   spin_unlock_bh(>np_thread_lock);
complete(>np_restart_comp);
+   return 1;
} else if (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) {
spin_unlock_bh(>np_thread_lock);
goto exit;
@@ -1278,7 +1280,8 @@ static int __iscsi_target_login_thread(struct iscsi_np 
*np)
goto exit;
} else if (rc < 0) {
spin_lock_bh(>np_thread_lock);
-   if (np->np_thread_state == ISCSI_NP_THREAD_RESET) {
+   if (atomic_dec_if_positive(>np_reset_count) >= 0) {
+   np->np_thread_state = ISCSI_NP_THREAD_ACTIVE;
spin_unlock_bh(>np_thread_lock);
complete(>np_restart_comp);
iscsit_put_transport(conn->conn_transport);
diff --git a/include/target/iscsi/iscsi_target_core.h 
b/include/target/iscsi/iscsi_target_core.h
index 0ca1fb0..fb87d32 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ 

[PATCH] qla2xxx: Fix incorrect tcm_qla2xxx_free_cmd use during TMR ABORT (v2)

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch drops two incorrect usages of tcm_qla2xxx_free_cmd()
during TMR ABORT within tcm_qla2xxx_handle_data_work() and
tcm_qla2xxx_aborted_task(), which where attempting to dispatch
into workqueue context to do tcm_qla2xxx_complete_free() and
subsequently invoke transport_generic_free_cmd().

This is incorrect because during TMR ABORT target-core will
drop the outstanding se_cmd->cmd_kref references once it has
quiesced the se_cmd via transport_wait_for_tasks(), and in
the case of qla2xxx it should not attempt to do it's own
transport_generic_free_cmd() once the abort has occured.

As reported by Pascal, this was originally manifesting as a
BUG_ON(cmd->cmd_in_wq) in qlt_free_cmd() during TMR ABORT,
with a LIO backend that had sufficently high enough WRITE
latency to trigger a host side TMR ABORT_TASK.

(v2: Drop the qla_tgt_cmd->write_pending_abort_comp changes,
 as they will be addressed in a seperate series)

Reported-by: Pascal de Bruijn <p.debru...@unilogic.nl>
Tested-by: Pascal de Bruijn <p.debru...@unilogic.nl>
Cc: Pascal de Bruijn <p.debru...@unilogic.nl>
Reported-by: Lukasz Engel <lukasz.en...@softax.pl>
Cc: Lukasz Engel <lukasz.en...@softax.pl>
Acked-by: Himanshu Madhani <himanshu.madh...@cavium.com>
Cc: Quinn Tran <quinn.t...@cavium.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 30 --
 1 file changed, 30 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b20da0d..3f82ea1 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -500,7 +500,6 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, 
struct qla_tgt_cmd *cmd,
 static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
 {
struct qla_tgt_cmd *cmd = container_of(work, struct qla_tgt_cmd, work);
-   unsigned long flags;
 
/*
 * Ensure that the complete FCP WRITE payload has been received.
@@ -508,17 +507,6 @@ static void tcm_qla2xxx_handle_data_work(struct 
work_struct *work)
 */
cmd->cmd_in_wq = 0;
 
-   spin_lock_irqsave(>cmd_lock, flags);
-   cmd->data_work = 1;
-   if (cmd->aborted) {
-   cmd->data_work_free = 1;
-   spin_unlock_irqrestore(>cmd_lock, flags);
-
-   tcm_qla2xxx_free_cmd(cmd);
-   return;
-   }
-   spin_unlock_irqrestore(>cmd_lock, flags);
-
cmd->qpair->tgt_counters.qla_core_ret_ctio++;
if (!cmd->write_data_transferred) {
/*
@@ -765,31 +753,13 @@ static void tcm_qla2xxx_queue_tm_rsp(struct se_cmd 
*se_cmd)
qlt_xmit_tm_rsp(mcmd);
 }
 
-#define DATA_WORK_NOT_FREE(_cmd) (_cmd->data_work && !_cmd->data_work_free)
 static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 {
struct qla_tgt_cmd *cmd = container_of(se_cmd,
struct qla_tgt_cmd, se_cmd);
-   unsigned long flags;
 
if (qlt_abort_cmd(cmd))
return;
-
-   spin_lock_irqsave(>cmd_lock, flags);
-   if ((cmd->state == QLA_TGT_STATE_NEW)||
-   ((cmd->state == QLA_TGT_STATE_DATA_IN) &&
-   DATA_WORK_NOT_FREE(cmd))) {
-   cmd->data_work_free = 1;
-   spin_unlock_irqrestore(>cmd_lock, flags);
-   /*
-* cmd has not reached fw, Use this trigger to free it.
-*/
-   tcm_qla2xxx_free_cmd(cmd);
-   return;
-   }
-   spin_unlock_irqrestore(>cmd_lock, flags);
-   return;
-
 }
 
 static void tcm_qla2xxx_clear_sess_lookup(struct tcm_qla2xxx_lport *,
-- 
1.9.1

Re: [PATCH] tcmu: clean up the scatter helper

[Nicholas A. Bellinger]

On Thu, 2017-07-13 at 14:33 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> Add some comments to make the scatter code to be more readable.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 30 +-
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 

Applied to target-pending/for-next.

Thanks Xiubo + MNC.

Re: [PATCH] tcmu: Fix possible to/from address overflow when doing the memcpy

[Nicholas A. Bellinger]

On Wed, 2017-07-12 at 15:51 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> For most case the sg->length equals to PAGE_SIZE, so this bug won't
> be triggered. Otherwise this will crash the kernel, for example when
> all segments' sg->length equal to 1K.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 

Applied to target-pending/master.

Thankx Xiubo + MNC.

Re: [PATCHv2] tcmu: Add fifo type waiter list support to avoid starvation

[Nicholas A. Bellinger]

Hi Xiubo,

Apologies for the delayed response.  Comments below.

On Wed, 2017-07-12 at 15:16 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> The fifo type waiter list will hold the udevs who are waiting for the
> blocks from the data global pool. The unmap thread will try to feed the
> first udevs in waiter list, if the global free blocks available are
> not enough, it will stop traversing the list and abort waking up the
> others.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 104 
> --
>  1 file changed, 88 insertions(+), 16 deletions(-)
> 

Applied to target-pending/for-next.

Thanks Xiubo + MNC.

[GIT PULL] target updates for v4.13-rc1

[Nicholas A. Bellinger]

Hi Linus,

Here are the target-pending updates for v4.13-rc1.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next

Note there is a qla2xxx conflict with scsi.git as reported by SFR, which
should be straight-forward to resolve:

https://lkml.org/lkml/2017/6/28/45

Note there was two additional qla2xxx conflicts with scsi.git:

https://lkml.org/lkml/2017/7/3/22
https://lkml.org/lkml/2017/7/3/23

However, the patch that caused these has since been reverted:

  55dd8cf Revert "qla2xxx: Fix incorrect tcm_qla2xxx_free_cmd use during TMR 
ABORT"

as it caused other problems, so these conflicts can be ignored.

Attached is v4.13-rc1-resolution.diff if there are any questions wrt to
how the resolution should look.  Adding Himanshu + Quinn CC'.

That said, it's been usually busy for summer, with most of the efforts
centered around TCMU developments and various target-core + fabric
driver bug fixing activities.  Not particularly large in terms of LoC,
but lots of smaller patches from many different folks.

The highlights include:

- ibmvscsis logical partition manager support (Michael Cyr + Bryant Ly)
- Convert target/iblock WRITE_SAME to blkdev_issue_zeroout (hch + nab)
- Add support for TMR percpu LUN reference counting (nab)
- Fix a potential deadlock between EXTENDED_COPY and iscsi shutdown (Bart)
- Fix COMPARE_AND_WRITE caw_sem leak during se_cmd quiesce (Jiang Yi)
- Fix TMCU module removal (Xiubo Li)
- Fix iser-target OOPs during login failure (Andrea Righi + Sagi)
- Breakup target-core free_device backend driver callback (mnc)
- Perform TCMU add/delete/reconfig synchronously (mnc)
- Fix TCMU  multiple UIO open/close sequences (mnc)
- Fix TCMU CHECK_CONDITION sense handling (mnc)
- Fix target-core SAM_STAT_BUSY + TASK_SET_FULL handling (mnc + nab)
- Introduce TYPE_ZBC support in PSCSI (Damien Le Moal)
- Fix possible TCMU memory leak + OOPs when recalculating cmd base size
  (Xiubo Li + Bryant Ly + Damien Le Moal + mnc)
- Add login_keys_workaround attribute for non RFC initiators
  (Robert LeBlanc + Arun Easi + nab)

Thank you,

--nab

Bart Van Assche (17):
  target: Use symbolic value for WRITE_VERIFY_16
  target: Remove se_device.dev_list
  target: Fix transport_init_se_cmd()
  target: Use {get,put}_unaligned_be*() instead of open coding these
functions
  target: Fix a deadlock between the XCOPY code and iSCSI session
shutdown
  IB/srpt: Make a debug statement in srpt_abort_cmd() more informative
  xen/scsiback: Fix a TMR related use-after-free
  xen/scsiback: Replace a waitqueue and a counter by a completion
  xen/scsiback: Make TMF processing slightly faster
  target: Introduce a function that shows the command state
  target/tcm_loop: Merge struct tcm_loop_cmd and struct tcm_loop_tmr
  target/tcm_loop: Replace a waitqueue and a counter by a completion
  target/tcm_loop: Use target_submit_tmr() instead of open-coding this
function
  target/tcm_loop: Make TMF processing slightly faster
  target/iscsi: Remove second argument of __iscsit_free_cmd()
  target/iscsi: Simplify iscsit_free_cmd()
  target/iscsi: Remove dead code from iscsit_process_scsi_cmd()

Bryant G. Ly (7):
  ibmvscsis: Use tpgt passed in by user
  tcmu: Support emulate_write_cache
  tcmu: Add netlink for device reconfiguration
  tcmu: Make dev_size configurable via userspace
  tcmu: Make dev_config configurable
  tcmu: Add Type of reconfig into netlink
  tcmu: Fix dev_config_store

Byungchul Park (1):
  vhost/scsi: Don't reinvent the wheel but use existing llist API

Colin Ian King (2):
  tcmu: make array tcmu_attrib_attrs static const
  target: make device_mutex and device_list static

Damien Le Moal (2):
  target: Use macro for WRITE_VERIFY_32 operation codes
  target: pscsi: Introduce TYPE_ZBC support

Gustavo A. R. Silva (1):
  target: remove dead code

Jiang Yi (2):
  target: reject COMPARE_AND_WRITE if emulate_caw is not set
  target: Fix COMPARE_AND_WRITE caw_sem leak during se_cmd quiesce

Michael Cyr (1):
  ibmvscsis: Enable Logical Partition Migration Support

Mike Christie (17):
  tcmu: reconfigure netlink attr changes
  target: break up free_device callback
  target: use idr for se_device dev index
  target: add helper to find se_device by dev_index
  tcmu: perfom device add, del and reconfig synchronously
  target: add helper to iterate over devices
  xcopy: loop over devices using idr helper
  target: remove g_device_list
  tcmu: drop configured check in destroy
  tcmu: fix multiple uio open/close sequences
  target: do not require a transport_complete for
SCF_TRANSPORT_TASK_SENSE
  target: add helper to copy sense to se_cmd buffer
  tcmu: fix sense handling during completion
  pscsi: finish cmd processing from pscsi_req_done
  target: remove transport_complete
  target: fix SAM_STAT_BUSY/TASK_SET_FULL handling
  target: export lio pgr/alua support as device attr

Nicholas Bellinger (12):
  target/iblock: Convert WRITE_SAME to blkdev_iss

Re: [PATCH] iscsi-target: Reject immediate data underflow larger than SCSI transfer length

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 16:17 +, Bart Van Assche wrote:
> On Tue, 2017-07-11 at 00:22 -0700, Nicholas A. Bellinger wrote:
> > So rejecting this case as already done in commit abb85a9b51 is the
> > correct approach for >= v4.3.y.
> 
> Hello Nic,
> 
> I hope that you agree that the current target_cmd_size_check() implementation
> is complicated and ugly. Patch 30/33 of the patch series I referred to in my
> e-mail removes a significant number of lines of code from that function. So
> my patch series not only makes target_cmd_size_check() easier to maintain and
> to verify but it makes that function also faster. Hence please reconsider the
> approach from my patch series. For patch 30/33, see also
> https://www.spinics.net/lists/target-devel/msg15384.html.

For reference, here is the patch your advocating:

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index c28e3b58150b..6cd49fe578a7 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1164,23 +1164,6 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int 
size)
" %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;
-   }
-   /*
-* Reject READ_* or WRITE_* with overflow/underflow for
-* type SCF_SCSI_DATA_CDB.
-*/
-   if (dev->dev_attrib.block_size != 512)  {
-   pr_err("Failing OVERFLOW/UNDERFLOW for LBA op"
-   " CDB on non 512-byte sector setup subsystem"
-   " plugin: %s\n", dev->transport->name);
-   /* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
-   return TCM_INVALID_CDB_FIELD;
-   }
/*
 * For the overflow case keep the existing fabric provided
 * ->data_length.  Otherwise for the underflow case, reset

The original code is not 'complicated' or 'ugly', and the proposed
change doesn't make anything 'faster' nor removes a 'significant' number
of LoC.

So no, I don't buy any of that line of reasoning.  ;-)

It comes down to two items.  First, if SCF_SCSI_DATA_CDBs with
underflow/overflow will be allowed, and second the block_size != 512
check.

For the former, I've still never seen a host environment in the wild
over the last 15 years that generates underflow/overflow for DATA CDBs
with an LBA.  So I'm reluctant to randomly allow this for all cases and
fabrics, considering no host environment actually requires it.

That said, I do understand libiscsi generates this for WRITE_VERIFY, but
I'm still undecided if that's a good enough a reason to enable it for
all cases in upstream.

For the latter item, it's fine to drop the legacy block_size != 512
check, and I'll take a patch for that separate from the other bit.

Re: [PATCH v2 01/15] qla2xxx: Combine Active command arrays.

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 23:43 +, Bart Van Assche wrote:
> On Tue, 2017-06-13 at 20:47 -0700, Himanshu Madhani wrote:
> >  typedef struct srb {
> > +   /*
> > +* Do not move cmd_type field, it needs to
> > +* line up with qla_tgt_cmd->cmd_type
> > +*/
> > +   uint8_t cmd_type;
> > +   uint8_t pad[3];
> 
> Hello Himanshu,
> 
> Had I understood correctly that you had promised to rework the command
> array merging such that a union is used instead of requiring certain
> fields to be present at certain offsets (see also
> https://www.spinics.net/lists/target-devel/msg15591.html)?
> 

Yeah, as discussed previously it was not a blocker for merge.

Re: [PATCH] iscsi-target: Add login_keys_workaround attribute for non RFC initiators

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 23:38 +, Bart Van Assche wrote:
> On Fri, 2017-07-07 at 22:24 +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <n...@linux-iscsi.org>
> > 
> > This patch re-introduces part of a long standing login workaround that
> > was recently dropped by:
> > 
> >   commit 1c99de981f30b3e7868b8d20ce5479fa1c0fea46
> >   Author: Nicholas Bellinger <n...@linux-iscsi.org>
> >   Date:   Sun Apr 2 13:36:44 2017 -0700
> > 
> >   iscsi-target: Drop work-around for legacy GlobalSAN initiator
> > 
> > Namely, the workaround for FirstBurstLength ended up being required by
> > Mellanox Flexboot PXE boot ROMs as reported by Robert.
> > 
> > So this patch re-adds the work-around for FirstBurstLength within
> > iscsi_check_proposer_for_optional_reply(), and makes the key optional
> > to respond when the initiator does not propose, nor respond to it.
> > 
> > Also as requested by Arun, this patch introduces a new TPG attribute
> > named 'login_keys_workaround' that controls the use of both the
> > FirstBurstLength workaround, as well as the two other existing
> > workarounds for gPXE iSCSI boot client.
> > 
> > By default, the workaround is enabled with login_keys_workaround=1,
> > since Mellanox FlexBoot requires it, and Arun has verified the Qlogic
> > MSFT initiator already proposes FirstBurstLength, so it's uneffected
> > by this re-adding this part of the original work-around.
> 
> Hello Nick,
> 
> The new configfs attribute ("login_keys_workaround") may confuse users - for
> someone who has not followed this e-mail thread it can take a long time
> before they figure out that they need to set this configfs attribute.

It's enabled by default, so there is nothing a user has to explicitly
change in order all hosts to 'just work'.

The only reason the attribute was added was by request of Arun, so if
some future initiator doesn't proposed the keys controlled by the
work-around, and still attempts to respond they can at least get
something working w/o code change.

>  Have
> you considered to let the iSCSI target driver figure out whether or not that
> variable has to be set, e.g. by looking up the initiator IQN in a list?
> 

Given InitiatorName is end user configurable, trying to do a workaround
based on IQN regexs is error prone, at best.

Also, since the FirstBurstLength work-around this patch re-adds had
already been in place for the better part of 8 years, the risk of
interopt issues is almost non existent.

Re: [PATCH] tcmu: clean up the code and with one small fix

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 18:06 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> Remove useless blank line and code and at the same time add one error
> path to catch the errors.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 24 +++-
>  1 file changed, 11 insertions(+), 13 deletions(-)

Applied.

Thanks Xiubo.

Re: [PATCHv2] tcmu: Fix possbile memory leak when recalculating the cmd base size

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 17:59 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> For all the entries allocated from the ring cmd area, the memory is
> something like the stack memory, which will always reserve the old
> data, so the entry->req.iov_bidi_cnt maybe none zero.
> 
> On some environments, the crash could be reporduce very easy and some
> not. The following is the crash core trace:
> 
> [  240.143969] CPU: 0 PID: 1285 Comm: iscsi_trx Not tainted
> 4.12.0-rc1+ #3
> [  240.150607] Hardware name: ASUS All Series/H87-PRO, BIOS 2104
> 10/28/2014
> [  240.157331] task: 8807de4f5800 task.stack:
> c900047dc000
> [  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
> [  240.167377] RSP: 0018:c900047dfc68 EFLAGS: 00010202
> [  240.172621] RAX: c9065db85540 RBX: 8807f798 RCX:
> 0010
> [  240.179771] RDX: 0010 RSI: 8807de574fe0 RDI:
> c9065db85540
> [  240.186930] RBP: c900047dfd30 R08: 8807de41b000 R09:
> 
> [  240.194088] R10: 0040 R11: 8807e9b726f0 R12:
> 0006565726b0
> [  240.201246] R13: c90007612ea0 R14: 00065657d540 R15:
> 
> [  240.208397] FS:  ()
> GS:88081fa0()
> knlGS:
> [  240.216510] CS:  0010 DS:  ES:  CR0: 80050033
> [  240.80] CR2: c9065db85540 CR3: 01c0f000 CR4:
> 001406f0
> [  240.229430] Call Trace:
> [  240.231887]  ? tcmu_queue_cmd+0x83c/0xa80
> [  240.235916]  ? target_check_reservation+0xcd/0x6f0
> [  240.240725]  __target_execute_cmd+0x27/0xa0
> [  240.244918]  target_execute_cmd+0x232/0x2c0
> [  240.249124]  ? __local_bh_enable_ip+0x64/0xa0
> [  240.253499]  iscsit_execute_cmd+0x20d/0x270
> [  240.257693]  iscsit_sequence_cmd+0x110/0x190
> [  240.261985]  iscsit_get_rx_pdu+0x360/0xc80
> [  240.267565]  ? iscsi_target_rx_thread+0x54/0xd0
> [  240.273571]  iscsi_target_rx_thread+0x9a/0xd0
> [  240.279413]  kthread+0x113/0x150
> [  240.284120]  ? iscsi_target_tx_thread+0x1e0/0x1e0
> [  240.290297]  ? kthread_create_on_node+0x40/0x40
> [  240.296297]  ret_from_fork+0x2e/0x40
> [  240.301332] Code: 90 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48
> 89 d1 48
> c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48
> 89 f8 48
> 89 d1  a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e
> 40 38
> [  240.321751] RIP: memcpy_erms+0x6/0x10 RSP: c900047dfc68
> [  240.328838] CR2: c9065db85540
> [  240.333667] ---[ end trace b7e5354cfb54d08b ]---
> 
> To fix this, just memset all the entry memory before using it, and
> also to be more readable we adjust the bidi code.
> 
> Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
>   memories)
> Reported-by: Bryant G. Ly 
> Tested-by: Damien Le Moal 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 

Applied, with a CC' to v4.12.y and slightly updated patch subject.

Thanks Xiubo, Bryant, Damien and MNC!

Re: [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size

[Nicholas A. Bellinger]

On Tue, 2017-07-11 at 09:24 +, Damien Le Moal wrote:
> Xiubo,
> 
> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
> my target without crashing the kernel.
> 
> Please see some comments/nitpicks below.
> 
> Otherwise, please feel free to add my "tested-by"
> 

Great, thanks for the quick turn-around.
 
Xiubo, please pick-up Damien's extra bits and repost for v2.  Also,
please update the patch topic + commit log to mention it addresses a
general protection fault OOPs, so others can find it easily when looking
through git log.

Damien's OOPs would probably be the best one to include in the commit
log, since Bryant's was on POWER.  ;)

Btw, since it's a regression fix related to commit fe25cc34795, I assume
it should be CC'ed to stable for v4.12.y, right..?

Re: [PATCH] iscsi-target: Add login_keys_workaround attribute for non RFC initiators

[Nicholas A. Bellinger]

Hey Robert,

Any chance to test this with your Flexboot PXE setup..?

Please give this a spin ASAP to verify it addresses the regression you
reported earlier, wrt FirstBurstLength not being proposed nor responded
to using Flexboot PXE.

Thank you.

On Fri, 2017-07-07 at 22:24 +, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> This patch re-introduces part of a long standing login workaround that
> was recently dropped by:
> 
>   commit 1c99de981f30b3e7868b8d20ce5479fa1c0fea46
>   Author: Nicholas Bellinger <n...@linux-iscsi.org>
>   Date:   Sun Apr 2 13:36:44 2017 -0700
> 
>   iscsi-target: Drop work-around for legacy GlobalSAN initiator
> 
> Namely, the workaround for FirstBurstLength ended up being required by
> Mellanox Flexboot PXE boot ROMs as reported by Robert.
> 
> So this patch re-adds the work-around for FirstBurstLength within
> iscsi_check_proposer_for_optional_reply(), and makes the key optional
> to respond when the initiator does not propose, nor respond to it.
> 
> Also as requested by Arun, this patch introduces a new TPG attribute
> named 'login_keys_workaround' that controls the use of both the
> FirstBurstLength workaround, as well as the two other existing
> workarounds for gPXE iSCSI boot client.
> 
> By default, the workaround is enabled with login_keys_workaround=1,
> since Mellanox FlexBoot requires it, and Arun has verified the Qlogic
> MSFT initiator already proposes FirstBurstLength, so it's uneffected
> by this re-adding this part of the original work-around.
> 
> Reported-by: Robert LeBlanc <rob...@leblancnet.us>
> Cc: Robert LeBlanc <rob...@leblancnet.us>
> Cc: Arun Easi <arun.e...@cavium.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/target/iscsi/iscsi_target_configfs.c   |  2 ++
>  drivers/target/iscsi/iscsi_target_nego.c   |  6 ++--
>  drivers/target/iscsi/iscsi_target_parameters.c | 41 
> ++
>  drivers/target/iscsi/iscsi_target_parameters.h |  2 +-
>  drivers/target/iscsi/iscsi_target_tpg.c| 19 
>  drivers/target/iscsi/iscsi_target_tpg.h|  1 +
>  include/target/iscsi/iscsi_target_core.h   |  9 ++
>  7 files changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
> b/drivers/target/iscsi/iscsi_target_configfs.c
> index 535a8e0..0dd4c45 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -781,6 +781,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
> *se_nacl,
>  DEF_TPG_ATTRIB(t10_pi);
>  DEF_TPG_ATTRIB(fabric_prot_type);
>  DEF_TPG_ATTRIB(tpg_enabled_sendtargets);
> +DEF_TPG_ATTRIB(login_keys_workaround);
>  
>  static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
>   _tpg_attrib_attr_authentication,
> @@ -796,6 +797,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
> *se_nacl,
>   _tpg_attrib_attr_t10_pi,
>   _tpg_attrib_attr_fabric_prot_type,
>   _tpg_attrib_attr_tpg_enabled_sendtargets,
> + _tpg_attrib_attr_login_keys_workaround,
>   NULL,
>  };
>  
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c 
> b/drivers/target/iscsi/iscsi_target_nego.c
> index 96df63f..7a6751f 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -864,7 +864,8 @@ static int iscsi_target_handle_csg_zero(
>   SENDER_TARGET,
>   login->rsp_buf,
>   >rsp_length,
> - conn->param_list);
> + conn->param_list,
> + conn->tpg->tpg_attrib.login_keys_workaround);
>   if (ret < 0)
>   return -1;
>  
> @@ -934,7 +935,8 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn 
> *conn, struct iscsi_log
>   SENDER_TARGET,
>   login->rsp_buf,
>   >rsp_length,
> - conn->param_list);
> + conn->param_list,
> + conn->tpg->tpg_attrib.login_keys_workaround);
>   if (ret < 0) {
>   iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
>   ISCSI_LOGIN_STATUS_INIT_ERR);
> diff --git a/drivers/target/iscsi/iscsi_target_parameters.c 
> b/drivers/target/iscsi/iscsi_target_parameters.c
> index fce6276..caab104 100644
> --- a/drivers/target/iscsi/iscsi_target_parameters.c
> +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> @@ -765,7 +765,8 @@ static int iscsi_check_for_aut

Re: [PATCH] scsi: qla2xxx: Off by one in qlt_ctio_to_cmd()

[Nicholas A. Bellinger]

On Mon, 2017-07-10 at 11:47 +0300, Dan Carpenter wrote:
> There are "req->num_outstanding_cmds" elements in the
> req->outstanding_cmds[] array so the > here should be >=.
> 
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c 
> b/drivers/scsi/qla2xxx/qla_target.c
> index 6e4794367e0b..ecd1a95511f9 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -3728,7 +3728,7 @@ static struct qla_tgt_cmd *qlt_ctio_to_cmd(struct 
> scsi_qla_host *vha,
>   h &= QLA_CMD_HANDLE_MASK;
>  
>   if (h != QLA_TGT_NULL_HANDLE) {
> - if (unlikely(h > req->num_outstanding_cmds)) {
> + if (unlikely(h >= req->num_outstanding_cmds)) {
>   ql_dbg(ql_dbg_tgt, vha, 0xe052,
>   "qla_target(%d): Wrong handle %x received\n",
>   vha->vp_idx, handle);

Nice catch.

Reviewed-by: Nicholas Bellinger <n...@linux-iscsi.org>

Re: [PATCH] tcmu: Fix possible overflow for memcpy address in iovec

[Nicholas A. Bellinger]

Hey Xiubo,

On Tue, 2017-07-11 at 16:04 +0800, Xiubo Li wrote:
> Hi All
> 
> Please ignore about this patch.
> 
> Just my mistake.
> 
> Sorry.
> 

Damien (CC'ed) has been observing something similar atop the latest
target-pending/for-next with his user-space ZBC backend:

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

Just curious, are you going to re-send a different patch to address
this..?

Is there anything that he can test to verify it's the same bug..?

> 
> Brs
> 
> Xiubo
> 
> 
> 
> On 2017年07月11日 15:40, lixi...@cmss.chinamobile.com wrote:
> > From: Xiubo Li 
> >
> > Before the data area dynamic grow patches, though the overflow
> > bug was already exist, since the data area memories are all
> > preallocated, so there mostly won't any bad page fault core
> > trace produced.
> >
> > The dynamic grow patches will only allocate and map the block
> > needed in data area, so when memcpy overflow, the system will
> > die.
> >
> > [  367.864705] [c000fc657340] [c00d220c] do_exit+0x79c/0xcf0
> > [  367.864710] [c000fc657410] [c00249a4] die+0x314/0x470
> > [  367.864715] [c000fc6574a0] [c005425c] 
> > bad_page_fault+0xdc/0x150
> > [  367.864720] [c000fc657510] [c0008964] 
> > handle_page_fault+0x2c/0x30
> > [  367.864726] --- interrupt: 300 at memcpy_power7+0x20c/0x840
> > [  367.864726] LR = tcmu_queue_cmd+0x844/0xa80 [target_core_user]
> > [  367.864732] [c000fc657800] [d88916d8] 
> > tcmu_queue_cmd+0x768/0xa80 [target_core_user] (unreliable)
> > [  367.864746] [c000fc657940] [d2993184] 
> > __target_execute_cmd+0x54/0x150 [target_core_mod]
> > [  367.864758] [c000fc657970] [d2994708] 
> > transport_generic_new_cmd+0x158/0x2d0 [target_core_mod]
> > [  367.864770] [c000fc6579f0] [d29948e4] 
> > transport_handle_cdb_direct+0x64/0xd0 [target_core_mod]
> > [  367.864783] [c000fc657a60] [d2994af8] 
> > target_submit_cmd_map_sgls+0x1a8/0x320 [target_core_mod]
> > [  367.864796] [c000fc657af0] [d2994cb8] 
> > target_submit_cmd+0x48/0x60 [target_core_mod]
> > [  367.864803] [c000fc657b90] [d2a54bd0] 
> > ibmvscsis_scheduler+0x350/0x5c0 [ibmvscsis]
> > [  367.864808] [c000fc657c50] [c00f1c28] 
> > process_one_work+0x1e8/0x5b0
> > [  367.864813] [c000fc657ce0] [c00f2098] 
> > worker_thread+0xa8/0x650
> > [  367.864818] [c000fc657d80] [c00fa864] kthread+0x114/0x140
> > [  367.864823] [c000fc657e30] [c00098f0] 
> > ret_from_kernel_thread+0x5c/0x6c
> > [  367.864827] Instruction dump:
> > [  367.864829] 6042 7fe3fb78 4bfcd175 6000 4bfffecc 7c0802a6 
> > f8010010 6000
> > [  367.864838] 7c0802a6 f8010010 f821ffe1 e9230690  38210020 
> > e8010010 7c0803a6
> > [  367.864847] ---[ end trace 8d085df7e65f7d20 ]---
> > [  367.870358]
> > [  367.870362] Fixing recursive fault but reboot is needed!
> > [  388.859695] INFO: rcu_sched detected stalls on CPUs/tasks:
> > [  388.859717]  16-...: (0 ticks this GP) idle=7e3/140/0 
> > softirq=12245/12245 fqs=2622
> > [  388.859722]  (detected by 20, t=5252 jiffies, g=12458, c=12457, q=2904)
> > [  388.859744] Task dump for CPU 16:
> > [  388.859747] kworker/16:2D  0  6865 0 0x0800
> > [  388.859762] Call Trace:
> > [  388.859768] [c000fc6579a0] [c14ef090] 
> > sysctl_sched_migration_cost+0x0/0x4 (unreliable)
> > [  388.859778] [c000fc6579c0] [d8890c1c] 
> > tcmu_parse_cdb+0x2c/0x40 [target_core_user]
> > [  388.859782] [c000fc6579e0] [c000fc657a60] 0xc000fc657a60
> >
> > Reported-by: Bryant G. Ly 
> > Signed-off-by: Xiubo Li 
> > ---
> >   drivers/target/target_core_user.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/target/target_core_user.c 
> > b/drivers/target/target_core_user.c
> > index 930800c..86a845a 100644
> > --- a/drivers/target/target_core_user.c
> > +++ b/drivers/target/target_core_user.c
> > @@ -437,7 +437,7 @@ static int scatter_data_area(struct tcmu_dev *udev,
> > to_offset = get_block_offset_user(udev, dbi,
> > block_remaining);
> > offset = DATA_BLOCK_SIZE - block_remaining;
> > -   to = (void *)(unsigned long)to + offset;
> > +   to = (void *)((unsigned long)to + offset);
> >   
> > if (*iov_cnt != 0 &&
> > to_offset == iov_tail(udev, *iov)) {
> > @@ -510,7 +510,7 @@ static void gather_data_area(struct tcmu_dev *udev, 
> > struct tcmu_cmd *cmd,
> > copy_bytes = min_t(size_t, sg_remaining,
> > block_remaining);
> > offset = DATA_BLOCK_SIZE - block_remaining;
> > -   from = (void *)(unsigned long)from + offset;
> > + 

Re: [PATCH] iscsi-target: Reject immediate data underflow larger than SCSI transfer length

[Nicholas A. Bellinger]

Hi Bart,

On Thu, 2017-06-08 at 23:55 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2017-06-08 at 15:37 +, Bart Van Assche wrote:
> > On Thu, 2017-06-08 at 04:21 +, Nicholas A. Bellinger wrote:
> > > + /*
> > > +  * Check for underflow case where both EDTL and immediate data payload
> > > +  * exceeds what is presented by CDB's TRANSFER LENGTH, and what has
> > > +  * already been set in target_cmd_size_check() as se_cmd->data_length.
> > > +  *
> > > +  * For this special case, fail the command and dump the immediate data
> > > +  * payload.
> > > +  */
> > > + if (cmd->first_burst_len > cmd->se_cmd.data_length) {
> > > + cmd->sense_reason = TCM_INVALID_CDB_FIELD;
> > > + goto after_immediate_data;
> > > + }
> > 
> > A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048):
> > 
> >If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
> >SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
> >be set to the numerical value of (EDTL - SPDTL).
> > 
> > Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the
> > initiator is compliant with the iSCSI RFC.
> 
> Alas, the nuance of what this patch actually does was missed when you
> cut the context.
> 
> First, a bit of history.  LIO has rejected underflow for all WRITEs for
> the first ~12.5 years of RFC-3720, and in the context of iscsi-target
> mode there has never been a single host environment that ever once
> cared.
> 
> Since Roland's patch to allow underflow for control CDBs in v4.3+ opened
> this discussion for control CDBs with a WRITE payload in order to make
> MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become
> what control CDB WRITE underflow cases should we allow..?
> 
> The point with this patch is when a host is sending a underflow with a
> iscsi immediate data payload that exceeds SCSI transfer length, it's a
> bogus request with a garbage payload.  It's a garbage payload because
> the SCSI CDB itself obviously doesn't want anything to do it.
> 
> I'm very dubious of any host environment who's trying to do this for any
> CDB, and expects achieve expected results.
> 
> Of course, since v4.3+ normal overflow where SCSI transfer length
> matches the iscsi immediate data payload just works with or without this
> patch.
> 
> So to that extent, I'm going to push this patch as a defensive fix for
> v4.3+, to let those imaginary iscsi host environments know they being
> very, very naughty.
> 
> >  Please note that a fix that is
> > compliant with the iSCSI RFC is present in the following patch series: 
> > [PATCH
> > 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017
> > (https://www.spinics.net/lists/target-devel/msg15370.html).
> 
> So I might still consider this as a v4.13-rc item for control CDB
> underflow, but no way as stable material.
> 
> Also, there is certainly no way I'm going to allow a patch to randomly
> enable underflow/overflow for all WRITE non control CDBs tree-wide
> across all fabric drivers, because 1) no host environments actually care
> about this, and 2) it's still dangerous to do for all fabrics without
> some serious auditing.

After further consideration, I've decided against allowing iscsi-target
underflow with a immediate data payload larger than SCSI transfer
length.

Any host environment that attempts to send an underflow with a immediate
data payload larger than SCSI transfer length, expects the target to
automatically truncate immediate data, and still return GOOD status is
completely bogus.  Any host that attempts this is buggy, and needs to be
fixed.

This is because for the last ~12 years of RFC-3720:

  - There has never been a host environment in the wild that exhibits 
this behavior.
  - There has never been a conformance suite which expects this 
behavior.

So rejecting this case as already done in commit abb85a9b51 is the
correct approach for >= v4.3.y.

Of course, the typical underflow scenario which Roland's v4.3.y commit
enabled, underflow where immediate data matches the SCSI transfer length
is supported for control CDBs.

That said, thanks for high-lighting this particular corner case, so it
could be fixed in >= v4.3.y.

Re: [PATCH v2 00/15] qla2xxx: Add Target Multiqueue support

[Nicholas A. Bellinger]

Hey guys,

Apologies for missing this earlier.  Comments below.

On Tue, 2017-06-13 at 20:47 -0700, Himanshu Madhani wrote:
> Hi Martin, Nic,
> 
> This patch series adds support for multiqueue for qla2xxx target mode driver.
> 
> This series depends on the seris applied to Martin's scsi/for-next branch
> (https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=for-next)
> 
> I've also added followig patch ("qla2xxx: Include Exchange offload/Extended
> Login into FW dump") which was dropped from earlier series for rework.
> 
> I am planning to send series for FC-NVME support as well which depends on this
> series, so I would like this series to be applied to scsi tree instead of
> target-pending for only this submission, as both series touches common code
> in qla2xxx driver. 
> 
> Please apply this series to for-next for inclusion in 4.13 merge window.
> 
> Note: Comment from Bart for patch#1 will be addresed as new patch after our 
>   FC-NVME series is posted. 
> 
> Changes from v1 --> v2
> 
> o Fixed 0-day kernel compile failure
> 
> Thanks,
> Himanshu 
> 
> Himanshu Madhani (1):
>   qla2xxx: Update driver version to 9.01.00.00-k
> 
> Quinn Tran (13):
>   qla2xxx: Combine Active command arrays.
>   qla2xxx: Preparation for Target MQ.
>   qla2xxx: Enable Target Multi Queue
>   qla2xxx: Add debug knob for user control workload
>   qla2xxx: Add fw_started flags to qpair
>   qla2xxx: move fields from qla_hw_data to qla_qpair
>   qla2xxx: use shadow register for ISP27XX
>   qla2xxx: Add function call to qpair for door bell
>   qla2xxx: Add debug logging routine for qpair
>   qla2xxx: Remove unused tgt_enable_64bit_addr flag
>   qla2xxx: Remove datasegs_per_cmd and datasegs_per_cont field
>   qla2xxx: Move target stat counters from vha to qpair.
>   qla2xxx: Include Exchange offload/Extended Login into FW dump
> 
> Sawan Chandak (1):
>   qla2xxx: Fix mailbox failure while deleting Queue pairs
> 
>  drivers/scsi/qla2xxx/qla_attr.c|4 +-
>  drivers/scsi/qla2xxx/qla_dbg.c |  150 ++
>  drivers/scsi/qla2xxx/qla_dbg.h |   17 +
>  drivers/scsi/qla2xxx/qla_def.h |  119 -
>  drivers/scsi/qla2xxx/qla_dfs.c |  137 -
>  drivers/scsi/qla2xxx/qla_gbl.h |   19 +-
>  drivers/scsi/qla2xxx/qla_init.c|   53 +-
>  drivers/scsi/qla2xxx/qla_inline.h  |   44 ++
>  drivers/scsi/qla2xxx/qla_iocb.c|   32 +-
>  drivers/scsi/qla2xxx/qla_isr.c |   93 +++-
>  drivers/scsi/qla2xxx/qla_mid.c |   44 +-
>  drivers/scsi/qla2xxx/qla_os.c  |  156 --
>  drivers/scsi/qla2xxx/qla_target.c  | 1028 
> +---
>  drivers/scsi/qla2xxx/qla_target.h  |   50 +-
>  drivers/scsi/qla2xxx/qla_version.h |4 +-
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |8 +-
>  16 files changed, 1348 insertions(+), 610 deletions(-)
> 

If it hasn't already been merged to MKP's tree, then:

Acked-by: Nicholas Bellinger <n...@linux-iscsi.org>

Btw, I don't think my missing ACK should hold up qla2xxx patches from
being merged in the future, especially ones that are exposing new
qla2xxx specific functionality.

The only ones that would really need my ACK are ones specific to
interaction with target-core.

[PATCH] iscsi-target: Add login_keys_workaround attribute for non RFC initiators

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch re-introduces part of a long standing login workaround that
was recently dropped by:

  commit 1c99de981f30b3e7868b8d20ce5479fa1c0fea46
  Author: Nicholas Bellinger <n...@linux-iscsi.org>
  Date:   Sun Apr 2 13:36:44 2017 -0700

  iscsi-target: Drop work-around for legacy GlobalSAN initiator

Namely, the workaround for FirstBurstLength ended up being required by
Mellanox Flexboot PXE boot ROMs as reported by Robert.

So this patch re-adds the work-around for FirstBurstLength within
iscsi_check_proposer_for_optional_reply(), and makes the key optional
to respond when the initiator does not propose, nor respond to it.

Also as requested by Arun, this patch introduces a new TPG attribute
named 'login_keys_workaround' that controls the use of both the
FirstBurstLength workaround, as well as the two other existing
workarounds for gPXE iSCSI boot client.

By default, the workaround is enabled with login_keys_workaround=1,
since Mellanox FlexBoot requires it, and Arun has verified the Qlogic
MSFT initiator already proposes FirstBurstLength, so it's uneffected
by this re-adding this part of the original work-around.

Reported-by: Robert LeBlanc <rob...@leblancnet.us>
Cc: Robert LeBlanc <rob...@leblancnet.us>
Cc: Arun Easi <arun.e...@cavium.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_configfs.c   |  2 ++
 drivers/target/iscsi/iscsi_target_nego.c   |  6 ++--
 drivers/target/iscsi/iscsi_target_parameters.c | 41 ++
 drivers/target/iscsi/iscsi_target_parameters.h |  2 +-
 drivers/target/iscsi/iscsi_target_tpg.c| 19 
 drivers/target/iscsi/iscsi_target_tpg.h|  1 +
 include/target/iscsi/iscsi_target_core.h   |  9 ++
 7 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
b/drivers/target/iscsi/iscsi_target_configfs.c
index 535a8e0..0dd4c45 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -781,6 +781,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
*se_nacl,
 DEF_TPG_ATTRIB(t10_pi);
 DEF_TPG_ATTRIB(fabric_prot_type);
 DEF_TPG_ATTRIB(tpg_enabled_sendtargets);
+DEF_TPG_ATTRIB(login_keys_workaround);
 
 static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
_tpg_attrib_attr_authentication,
@@ -796,6 +797,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
*se_nacl,
_tpg_attrib_attr_t10_pi,
_tpg_attrib_attr_fabric_prot_type,
_tpg_attrib_attr_tpg_enabled_sendtargets,
+   _tpg_attrib_attr_login_keys_workaround,
NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_nego.c 
b/drivers/target/iscsi/iscsi_target_nego.c
index 96df63f..7a6751f 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -864,7 +864,8 @@ static int iscsi_target_handle_csg_zero(
SENDER_TARGET,
login->rsp_buf,
>rsp_length,
-   conn->param_list);
+   conn->param_list,
+   conn->tpg->tpg_attrib.login_keys_workaround);
if (ret < 0)
return -1;
 
@@ -934,7 +935,8 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn 
*conn, struct iscsi_log
SENDER_TARGET,
login->rsp_buf,
>rsp_length,
-   conn->param_list);
+   conn->param_list,
+   conn->tpg->tpg_attrib.login_keys_workaround);
if (ret < 0) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
ISCSI_LOGIN_STATUS_INIT_ERR);
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c 
b/drivers/target/iscsi/iscsi_target_parameters.c
index fce6276..caab104 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -765,7 +765,8 @@ static int iscsi_check_for_auth_key(char *key)
return 0;
 }
 
-static void iscsi_check_proposer_for_optional_reply(struct iscsi_param *param)
+static void iscsi_check_proposer_for_optional_reply(struct iscsi_param *param,
+   bool keys_workaround)
 {
if (IS_TYPE_BOOL_AND(param)) {
if (!strcmp(param->value, NO))
@@ -773,19 +774,31 @@ static void 
iscsi_check_proposer_for_optional_reply(struct iscsi_param *param)
} else if (IS_TYPE_BOOL_OR(param)) {
if (!strcmp(param->value, YES))
SET_PSTATE_REPLY_OPTIONAL(param);
-/*
- * Required for gPXE iSCSI boot client
- */
-   if (!strcmp(

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Thu, 2017-07-06 at 23:05 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> > Nicholas,
> > 
> > On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > > Hey MNC & Co,
> > > 
> > > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> > >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> > >>> If the user request handler completed the request with a CHECK CONDITION
> > >>> status, tcmu_handle_completion() copies the command entry sense data
> > >>> into the session request structure sense data. However, the sense data
> > >>> length indicated by the field scsi_sense_length is not set and equal to
> > >>> 0, resulting in the copy being a no-op and failure to propagate the
> > >>> sense data back to the initiator. To fix this, properly set the session
> > >>> command sense data length and also set the session command
> > >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> > >>> sense data.
> > >>>
> > >>> Signed-off-by: Damien Le Moal <damien.lem...@wdc.com>
> > >>> ---
> > >>>  drivers/target/target_core_user.c | 4 +++-
> > >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/target/target_core_user.c 
> > >>> b/drivers/target/target_core_user.c
> > >>> index beb5f09..7426b4c 100644
> > >>> --- a/drivers/target/target_core_user.c
> > >>> +++ b/drivers/target/target_core_user.c
> > >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> > >>> *cmd, struct tcmu_cmd_entry *
> > >>> entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> > >>> } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> > >>> memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> > >>> -  se_cmd->scsi_sense_length);
> > >>> +  TRANSPORT_SENSE_BUFFER);
> > >>> +   se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> > >>> +   se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> > >>> } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > >>> /* Get Data-In buffer before clean up */
> > >>> gather_data_area(udev, cmd, true);
> > >>>
> > >>
> > >> I have a patch similar to this and 5/5 in my set:
> > >>
> > >> https://www.spinics.net/lists/target-devel/msg15430.html
> > >>
> > >> If yours gets merged first then I will build my set over them, so patch
> > >> looks ok to me.
> > >>
> > >> Reviewed-by: Mike Christie <mchri...@redhat.com>
> > > 
> > > The RFC patches above from May 31st weren't merged because I thought you
> > > where going to send out a second series..
> > 
> > My apologies. I have been busy with other things and could not get to that.
> > 

Oh btw, I was talking to MNC wrt to his original patch series, and not
yours.  :)

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> Nicholas,
> 
> On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > Hey MNC & Co,
> > 
> > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> >>> If the user request handler completed the request with a CHECK CONDITION
> >>> status, tcmu_handle_completion() copies the command entry sense data
> >>> into the session request structure sense data. However, the sense data
> >>> length indicated by the field scsi_sense_length is not set and equal to
> >>> 0, resulting in the copy being a no-op and failure to propagate the
> >>> sense data back to the initiator. To fix this, properly set the session
> >>> command sense data length and also set the session command
> >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> >>> sense data.
> >>>
> >>> Signed-off-by: Damien Le Moal <damien.lem...@wdc.com>
> >>> ---
> >>>  drivers/target/target_core_user.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/target/target_core_user.c 
> >>> b/drivers/target/target_core_user.c
> >>> index beb5f09..7426b4c 100644
> >>> --- a/drivers/target/target_core_user.c
> >>> +++ b/drivers/target/target_core_user.c
> >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> >>> *cmd, struct tcmu_cmd_entry *
> >>>   entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> >>>   } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> >>>   memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> >>> -se_cmd->scsi_sense_length);
> >>> +TRANSPORT_SENSE_BUFFER);
> >>> + se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> >>> + se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> >>>   } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> >>>   /* Get Data-In buffer before clean up */
> >>>   gather_data_area(udev, cmd, true);
> >>>
> >>
> >> I have a patch similar to this and 5/5 in my set:
> >>
> >> https://www.spinics.net/lists/target-devel/msg15430.html
> >>
> >> If yours gets merged first then I will build my set over them, so patch
> >> looks ok to me.
> >>
> >> Reviewed-by: Mike Christie <mchri...@redhat.com>
> > 
> > The RFC patches above from May 31st weren't merged because I thought you
> > where going to send out a second series..
> 
> My apologies. I have been busy with other things and could not get to that.
> 
> > 
> > https://www.spinics.net/lists/target-devel/msg15505.html
> > 
> > Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> > patches #1-6 for v4.13 now.  :)
> > 
> > Please resend patches #7-13 as post v4.13 items at your earliest
> > convenience.
> 
> I will retest first thing Monday the merge with Mikes series and send
> incremental fixes if any is needed.
> 

Great, thanks.

Everything including MNC's #1-6 and your #1-2 be pushed to
target-pending/for-next shortly.

Please use this as your base for testing.  :)

Re: [PATCH 5/5] target: core: Fix failed command sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:59 +0900, Damien Le Moal wrote:
> For a target device without a transport->transport_complete method
> defined (e.g. target_core_user), target_complete_cmd() will always
> result in a failed command completion being processed through target
> failure completion work even when the command failure comes from the
> target processing and has valid sense data (and hence does not require
> sense data emulation as done in the failure work processing). To ensure
> that the failed command sense data is propagated as indicated by the
> target, make sure that the normal "ok" work completion path is used by
> moving the command SCF_TRANSPORT_TASK_SENSE flag test out of the
> transport_complete defined conditional.
> 
> Signed-off-by: Damien Le Moal 
> ---
>  drivers/target/target_core_transport.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Per MNC, skipping this patch in favor of target_complete_cmd() checking
se_cmd->scsi_status:

https://www.spinics.net/lists/target-devel/msg15436.html

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> If the user request handler completed the request with a CHECK CONDITION
> status, tcmu_handle_completion() copies the command entry sense data
> into the session request structure sense data. However, the sense data
> length indicated by the field scsi_sense_length is not set and equal to
> 0, resulting in the copy being a no-op and failure to propagate the
> sense data back to the initiator. To fix this, properly set the session
> command sense data length and also set the session command
> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> sense data.
> 
> Signed-off-by: Damien Le Moal 
> ---
>  drivers/target/target_core_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Per MNC, skipping this patch in favor of tcmu_handle_completion() using
the new transport_copy_sense_to_cmd() helper.

https://www.spinics.net/lists/target-devel/msg15435.html

Re: [PATCH 3/5] target: pscsi: Fix sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> On completion of a request sent to the target backstore device,
> pscsi_req_done() calls target_complete_cmd() which in turn will execute
> pscsi_transport_complete(). In case of a failed request, this last
> function will copy the target request sense data to the initiator side
> request sense data. However, in pscsi_req_done(), the sense data is
> retreived from the request after calling target_complete_cmd(), which
> result in the sense data always conatining zeroes. Simply fix this by
> copying the sense data before calling target_complete_cmd().
> 
> Signed-off-by: Damien Le Moal 
> ---
>  drivers/target/target_core_pscsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

As per MNC, skipping this patch as the following has been applied that
makes pscsi_complete_cmd() use the new transport_copy_sense_to_cmd()
helper.

https://www.spinics.net/lists/target-devel/msg15433.html

Please let me know if you run into any issues.

Re: [PATCH 2/5] target: pscsi: Introduce TYPE_ZBC support

[Nicholas A. Bellinger]

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> TYPE_ZBC host managed zoned block devices are also block devices
> despite the non-standard device type (14h). Handle them similarly to
> regular TYPE_DISK devices.
> 
> Signed-off-by: Damien Le Moal 
> ---
>  drivers/target/target_core_pscsi.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)

Applied to target-pending/for-next.

Thanks.

Re: [PATCH 1/5] target: Use macro for WRITE_VERIFY_xx operation codes

[Nicholas A. Bellinger]

Hi Damien,

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> Add WRITE_VERIFY_32 definition to scsi prototypes and use this macro
> definition isntead of the hard coded value. Same for the already defined
> WRITE_VERIFY_16 command code.
> 
> Signed-off-by: Damien Le Moal 
> ---
>  drivers/target/target_core_device.c | 4 ++--
>  include/scsi/scsi_proto.h   | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)

Note the WRITE_VERIFY_16 macro usage has already been changed by
commit 500073bb23 in target-pending/for-next.

So applied to target-pending/for-next for the WRITE_VERIFY_32 specific
bits.

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

Hey MNC & Co,

On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> > If the user request handler completed the request with a CHECK CONDITION
> > status, tcmu_handle_completion() copies the command entry sense data
> > into the session request structure sense data. However, the sense data
> > length indicated by the field scsi_sense_length is not set and equal to
> > 0, resulting in the copy being a no-op and failure to propagate the
> > sense data back to the initiator. To fix this, properly set the session
> > command sense data length and also set the session command
> > SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> > sense data.
> > 
> > Signed-off-by: Damien Le Moal 
> > ---
> >  drivers/target/target_core_user.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/target/target_core_user.c 
> > b/drivers/target/target_core_user.c
> > index beb5f09..7426b4c 100644
> > --- a/drivers/target/target_core_user.c
> > +++ b/drivers/target/target_core_user.c
> > @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> > *cmd, struct tcmu_cmd_entry *
> > entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> > } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> > memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> > -  se_cmd->scsi_sense_length);
> > +  TRANSPORT_SENSE_BUFFER);
> > +   se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> > +   se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> > } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > /* Get Data-In buffer before clean up */
> > gather_data_area(udev, cmd, true);
> > 
> 
> I have a patch similar to this and 5/5 in my set:
> 
> https://www.spinics.net/lists/target-devel/msg15430.html
> 
> If yours gets merged first then I will build my set over them, so patch
> looks ok to me.
> 
> Reviewed-by: Mike Christie 

The RFC patches above from May 31st weren't merged because I thought you
where going to send out a second series..

https://www.spinics.net/lists/target-devel/msg15505.html

Since that hasn't been the case, I'll go ahead and merge the bugfixes in
patches #1-6 for v4.13 now.  :)

Please resend patches #7-13 as post v4.13 items at your earliest
convenience.

Apologies for the confusion.

Re: [PATCH] target: make device_mutex and device_list static

[Nicholas A. Bellinger]

On Wed, 2017-07-05 at 13:15 -0500, Mike Christie wrote:
> On 07/04/2017 03:44 AM, Colin King wrote:
> > From: Colin Ian King 
> > 
> > Variables device_mutex and device_list static are local to the source,
> > so make them static.
> > 
> > Cleans up sparse warnings:
> > "symbol 'device_list' was not declared. Should it be static?"
> > "symbol 'device_mutex' was not declared. Should it be static?"
> > 
> > Signed-off-by: Colin Ian King 
> > ---
> >  drivers/target/target_core_device.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_device.c 
> > b/drivers/target/target_core_device.c
> > index 3ae8fbf01bdf..bbcef3bc66c8 100644
> > --- a/drivers/target/target_core_device.c
> > +++ b/drivers/target/target_core_device.c
> > @@ -49,8 +49,8 @@
> >  #include "target_core_pr.h"
> >  #include "target_core_ua.h"
> >  
> > -DEFINE_MUTEX(device_mutex);
> > -LIST_HEAD(device_list);
> > +static DEFINE_MUTEX(device_mutex);
> > +static LIST_HEAD(device_list);
> >  static DEFINE_IDR(devices_idr);
> >  
> >  static struct se_hba *lun0_hba;
> > 
> 
> My fault. Thanks.
> 
> Reviewed-by: Mike Christie 

Applied.

Thanks Colin + MNC.

Re: [PATCH] tcmu: Fix flushing cmd entry dcache page

[Nicholas A. Bellinger]

On Fri, 2017-06-30 at 16:14 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> When feeding the tcmu's cmd ring, we need to flush the dcache page
> for the cmd entry to make sure these kernel stores are visible to
> user space mappings of that page.
> 
> For the none PAD cmd entry, this will be flushed at the end of the
> tcmu_queue_cmd_ring().
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied.

Thanks Xiubo + MNC.

Re: [PATCH v4 0/5] tcmu: Add Type of reconfig into netlink

[Nicholas A. Bellinger]

Hey MNC,

On Mon, 2017-06-12 at 01:43 -0500, Mike Christie wrote:
> On 06/11/2017 04:02 PM, Mike Christie wrote:
> > On 06/09/2017 01:11 AM, Nicholas A. Bellinger wrote:
> >> Hi Bryant & Co,
> >>
> >> On Tue, 2017-06-06 at 09:28 -0500, Bryant G. Ly wrote:
> >>> From: "Bryant G. Ly" <b...@us.ibm.com>
> >>>
> >>> This patch consists of adding a netlink to allow for reconfiguration
> >>> of a device in tcmu.
> >>>
> >>> It also changes and adds some attributes that are reconfigurable:
> >>> write_cache, device size, and device path.
> >>>
> >>> V2 - Fixes kfree in tcmu: Make dev_config configurable
> >>> V3 - Fixes spelling error
> >>> V4 - change strcpy to strlcpy for tcmu_dev_path_store and move
> >>>  tcmu_reconfig_type into target_core_user.h
> >>>
> >>>
> >>> Bryant G. Ly (5):
> >>>   tcmu: Support emulate_write_cache
> >>>   tcmu: Add netlink for device reconfiguration
> >>>   tcmu: Make dev_size configurable via userspace
> >>>   tcmu: Make dev_config configurable
> >>>   tcmu: Add Type of reconfig into netlink
> >>>
> >>>  drivers/target/target_core_user.c | 152 
> >>> --
> >>>  include/uapi/linux/target_core_user.h |   9 ++
> >>>  2 files changed, 155 insertions(+), 6 deletions(-)
> >>>
> >>
> >> AFAICT, it looks like all of the review comments have been addressed in
> >> -v4.
> >>
> >> Applied to target-pending/for-next, with MNC's (pseudo) Reviewed-by's
> >> added for #3-#5.
> >>
> >> Please let me know if anything else needs to be changed.
> >>
> > 
> > The patches look ok. Thanks. Could you just merge the attached patch
> > into "[PATCH v4 5/5] tcmu: Add Type of reconfig into netlink" or into
> > the patchset after it? It just makes some of the names a little less
> > generic and only returns the reconfig attr for reconfig commands.
> > 
> 
> Actually Nick, do not merge the last patch. I have a lot more fixes/changes.
> 
> Bryant, could you test and adapt your userspace patches for the attached
> patch build over Nicks for-next branch.
> 
> Basically, the patch just has use pass the value being reconfigured with
> the netlink event. Along the way, it fixes a couple bugs.
> 
> Nick, when we have tested the patch, then I can submit as a formal
> patchset, or you can fold into the existing patches or whatever you prefer.
> 

Looking at merging your -v4 patches series here next:

[PATCH V4 00/10] target/tcmu: make tcmu netlink ops sync
http://www.spinics.net/lists/target-devel/msg15706.html

Do you still want me to drop the patch from Bryant below as mentioned
earlier..?

tcmu: Add Type of reconfig into netlink
https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?h=for-next=28d44b983000754677155e46f6bdafc7b4d84213

Re: [PATCH][V2][target-devel-next] tcmu: make array tcmu_attrib_attrs static const

[Nicholas A. Bellinger]

On Tue, 2017-06-13 at 14:29 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> The array tcmu_attrib_attrs does not need to be in global scope, so make
> it static.
> 
> Cleans up sparse warning:
> "symbol 'tcmu_attrib_attrs' was not declared. Should it be static?"
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/target/target_core_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index afc1fd6bacaf..04fb3f720895 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1672,7 +1672,7 @@ static ssize_t tcmu_emulate_write_cache_store(struct 
> config_item *item,
>  }
>  CONFIGFS_ATTR(tcmu_, emulate_write_cache);
>  
> -struct configfs_attribute *tcmu_attrib_attrs[] = {
> +static struct configfs_attribute *tcmu_attrib_attrs[] = {
>   _attr_cmd_time_out,
>   _attr_dev_path,
>   _attr_dev_size,

Applied.

Thanks Colin.

Re: [PATCH] tcmu: Fix module removal due to stuck unmap_thread thread again

[Nicholas A. Bellinger]

On Thu, 2017-06-15 at 15:05 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> Because the unmap code just after the schdule() returned may take
> a long time and if the kthread_stop() is fired just when in this
> routine, the module removal maybe stuck too.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index beb5f09..203bff1 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1573,7 +1573,7 @@ static int unmap_thread_fn(void *data)
>   struct page *page;
>   int i;
>  
> - while (1) {
> + while (!kthread_should_stop()) {
>   DEFINE_WAIT(__wait);
>  
>   prepare_to_wait(_wait, &__wait, TASK_INTERRUPTIBLE);

Applied to target-pending/for-next.

Thanks Xiubo + MNC.

[GIT PULL] target fixes for v4.12-rc7

[Nicholas A. Bellinger]

Hi Linus,

Here are the target-pending fixes for v4.12-rc7 that have been queued up
for the last 2 weeks.  Please go ahead and pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git master

This includes:

- Fix a TMR related kref underflow detected by the recent refcount_t 
  conversion in upstream.
- Fix a iscsi-target corner case during explicit connection logout 
  timeout failure.
- Address last fallout in iscsi-target immediate data handling from
  v4.4 target-core now allowing control CDB payload underflow.

Thank you,

--nab

Nicholas Bellinger (3):
  target: Fix kref->refcount underflow in transport_cmd_finish_abort
  iscsi-target: Fix delayed logout processing greater than
SECONDS_FOR_LOGOUT_COMP
  iscsi-target: Reject immediate data underflow larger than SCSI
transfer length

 drivers/target/iscsi/iscsi_target.c| 22 --
 drivers/target/target_core_internal.h  |  2 +-
 drivers/target/target_core_tmr.c   | 16 
 drivers/target/target_core_transport.c |  9 ++---
 4 files changed, 35 insertions(+), 14 deletions(-)

Re: [PATCH] configfs: Fix race between create_link and configfs_rmdir

[Nicholas A. Bellinger]

On Thu, 2017-06-08 at 07:34 -0500, Bryant G. Ly wrote:
> > Thanks Nic,
> >
> > applied to the configfs-for-next tree.  I'm not entirely sure if we
> > should bother adding this to 4.12 or if it hits rarely enough?
> >
> It hits for us pretty often when we have a GPFS setup with 10 hosts and 1k+ 
> vms.
> 
> That is how we discovered the bug in the first place.
> 

Using a DATERA workload with 1K unique multi-tenant backend devices and
1K unique iscsi-target IQNs per node, I've never tripped across this
particular bug..

However, our userspace built atop rtslib is enforcing tenant shutdown of
individual rmdir(2) of /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/,
before rmdir(2) of /sys/kernel/config/target/core/$HBA/$DEV/ occurs.

Based on Bryant's original backtrace with targetcli, it looks like the
Novalink user-space is not enforcing this requirement across user-space
processes doing fabric port symlink and backend device shutdown.

That said it probably doesn't need a special v4.12-rc PULL, but based on
Bryant's feedback it certainly does deserve a stable CC'.

Re: [PATCH] iscsi-target: Reject immediate data underflow larger than SCSI transfer length

[Nicholas A. Bellinger]

On Thu, 2017-06-08 at 15:37 +, Bart Van Assche wrote:
> On Thu, 2017-06-08 at 04:21 +0000, Nicholas A. Bellinger wrote:
> > +   /*
> > +* Check for underflow case where both EDTL and immediate data payload
> > +* exceeds what is presented by CDB's TRANSFER LENGTH, and what has
> > +* already been set in target_cmd_size_check() as se_cmd->data_length.
> > +*
> > +* For this special case, fail the command and dump the immediate data
> > +* payload.
> > +*/
> > +   if (cmd->first_burst_len > cmd->se_cmd.data_length) {
> > +   cmd->sense_reason = TCM_INVALID_CDB_FIELD;
> > +   goto after_immediate_data;
> > +   }
> 
> A quote from the iSCSI RFC (https://tools.ietf.org/html/rfc5048):
> 
>If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
>SCSI Response PDU as specified in [RFC3720].  The Residual Count MUST
>be set to the numerical value of (EDTL - SPDTL).
> 
> Sorry but I don't think that sending TCM_INVALID_CDB_FIELD back to the
> initiator is compliant with the iSCSI RFC.

Alas, the nuance of what this patch actually does was missed when you
cut the context.

First, a bit of history.  LIO has rejected underflow for all WRITEs for
the first ~12.5 years of RFC-3720, and in the context of iscsi-target
mode there has never been a single host environment that ever once
cared.

Since Roland's patch to allow underflow for control CDBs in v4.3+ opened
this discussion for control CDBs with a WRITE payload in order to make
MSFT/FCP cert for PERSISTENT_RESERVE_OUT happy, the question has become
what control CDB WRITE underflow cases should we allow..?

The point with this patch is when a host is sending a underflow with a
iscsi immediate data payload that exceeds SCSI transfer length, it's a
bogus request with a garbage payload.  It's a garbage payload because
the SCSI CDB itself obviously doesn't want anything to do it.

I'm very dubious of any host environment who's trying to do this for any
CDB, and expects achieve expected results.

Of course, since v4.3+ normal overflow where SCSI transfer length
matches the iscsi immediate data payload just works with or without this
patch.

So to that extent, I'm going to push this patch as a defensive fix for
v4.3+, to let those imaginary iscsi host environments know they being
very, very naughty.

>  Please note that a fix that is
> compliant with the iSCSI RFC is present in the following patch series: [PATCH
> 00/33] SCSI target driver patches for kernel v4.13, 23 May 2017
> (https://www.spinics.net/lists/target-devel/msg15370.html).

So I might still consider this as a v4.13-rc item for control CDB
underflow, but no way as stable material.

Also, there is certainly no way I'm going to allow a patch to randomly
enable underflow/overflow for all WRITE non control CDBs tree-wide
across all fabric drivers, because 1) no host environments actually care
about this, and 2) it's still dangerous to do for all fabrics without
some serious auditing.

Re: [PATCH 00/15] qla2xxx: Add Target Multiqueue support

[Nicholas A. Bellinger]

Hi Himanshu & Quinn,

On Wed, 2017-06-07 at 14:43 -0700, Himanshu Madhani wrote:
> Hi Nic,
> 
> This patch series adds support for multiqueue for qla2xxx target mode driver.
> 
> I've also added patch ("qla2xxx: Include Exchange offload/Extended Login
> into FW dump") which was dropped from earlier series for rework.
> 
> Please apply this series to target-pending for inclusion in 4.13 merge window.

Bart's comments in patch #1 are reasonable to consider, but IMHO not a
blocker to initial merge.

That said, as-is this series doesn't apply (see below) to
target-pending/for-next, presumably due to dependencies on what's in
MKP's tree.

So I'm happy to accept a version that does apply.  ;)

Checking patch drivers/scsi/qla2xxx/qla_def.h...
Hunk #1 succeeded at 435 (offset -2 lines).
Hunk #2 succeeded at 3296 (offset -2 lines).
Hunk #3 succeeded at 4260 (offset -6 lines).
Checking patch drivers/scsi/qla2xxx/qla_gbl.h...
Hunk #1 succeeded at 833 (offset -1 lines).
Checking patch drivers/scsi/qla2xxx/qla_inline.h...
Hunk #1 succeeded at 262 (offset 12 lines).
Checking patch drivers/scsi/qla2xxx/qla_isr.c...
Hunk #2 succeeded at 2263 (offset -17 lines).
Hunk #3 succeeded at 2627 (offset -13 lines).
Hunk #4 succeeded at 2650 (offset -13 lines).
Hunk #5 succeeded at 2759 (offset -13 lines).
Checking patch drivers/scsi/qla2xxx/qla_os.c...
Hunk #1 succeeded at 1637 (offset -12 lines).
error: while searching for:
for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
sp = req->outstanding_cmds[cnt];
if (sp) {
/* Don't abort commands in adapter during EEH
 * recovery as it's not accessible/responding.
 */
if (GET_CMD_SP(sp) && !ha->flags.eeh_busy &&
(sp->type == SRB_SCSI_CMD)) {
/* Get a reference to the sp and drop 
the lock.
 * The reference ensures this 
sp->done() call
 * - and not the call in 
qla2xxx_eh_abort() -
 * ends the SCSI command (with result 
'res').
 */
sp_get(sp);

spin_unlock_irqrestore(>hardware_lock, flags);
status = 
qla2xxx_eh_abort(GET_CMD_SP(sp));
spin_lock_irqsave(>hardware_lock, 
flags);
/* Get rid of extra reference if 
immediate exit
 * from ql2xxx_eh_abort */
if (status == FAILED && 
(qla2x00_isp_reg_stat(ha)))
atomic_dec(>ref_count);
}
req->outstanding_cmds[cnt] = NULL;
sp->done(sp, res);
}
}
}

error: patch failed: drivers/scsi/qla2xxx/qla_os.c:1662
error: drivers/scsi/qla2xxx/qla_os.c: patch does not apply
Checking patch drivers/scsi/qla2xxx/qla_target.c...
Hunk #2 succeeded at 2278 (offset -11 lines).
Hunk #3 succeeded at 2317 (offset -11 lines).
Hunk #4 succeeded at 2336 (offset -11 lines).
Hunk #5 succeeded at 2881 (offset -12 lines).
Hunk #6 succeeded at 2987 (offset -12 lines).
Hunk #7 succeeded at 3675 (offset -6 lines).
Hunk #8 succeeded at 3719 (offset -6 lines).
Hunk #9 succeeded at 3754 (offset -6 lines).
Hunk #10 succeeded at 4035 (offset -6 lines).
Hunk #11 succeeded at 6622 (offset 27 lines).
Checking patch drivers/scsi/qla2xxx/qla_target.h...
Hunk #4 succeeded at 865 (offset 1 line).
Hunk #5 succeeded at 1090 (offset 2 lines).

Re: [PATCH v4 0/5] tcmu: Add Type of reconfig into netlink

[Nicholas A. Bellinger]

Hi Bryant & Co,

On Tue, 2017-06-06 at 09:28 -0500, Bryant G. Ly wrote:
> From: "Bryant G. Ly" 
> 
> This patch consists of adding a netlink to allow for reconfiguration
> of a device in tcmu.
> 
> It also changes and adds some attributes that are reconfigurable:
> write_cache, device size, and device path.
> 
> V2 - Fixes kfree in tcmu: Make dev_config configurable
> V3 - Fixes spelling error
> V4 - change strcpy to strlcpy for tcmu_dev_path_store and move
>  tcmu_reconfig_type into target_core_user.h
> 
> 
> Bryant G. Ly (5):
>   tcmu: Support emulate_write_cache
>   tcmu: Add netlink for device reconfiguration
>   tcmu: Make dev_size configurable via userspace
>   tcmu: Make dev_config configurable
>   tcmu: Add Type of reconfig into netlink
> 
>  drivers/target/target_core_user.c | 152 
> --
>  include/uapi/linux/target_core_user.h |   9 ++
>  2 files changed, 155 insertions(+), 6 deletions(-)
> 

AFAICT, it looks like all of the review comments have been addressed in
-v4.

Applied to target-pending/for-next, with MNC's (pseudo) Reviewed-by's
added for #3-#5.

Please let me know if anything else needs to be changed.

Re: [PATCH v1] ibmvscsis: Use tpgt passed in by user

[Nicholas A. Bellinger]

On Tue, 2017-06-06 at 15:45 -0500, Bryant G. Ly wrote:
> ibmvscsis always returned 0 for the tpg/tag, since it did not
> parse the value passed in by the user.
> 
> When functions like ALUA members exports the value, it will
> be incorrect because targetcli/rtslib starts the tpg numbering
> at 1.
> 
> Signed-off-by: Bryant G. Ly 
> Signed-off-by: Mike Christie 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 8 
>  1 file changed, 8 insertions(+)

Applied to target-pending/for-next.

Thanks Bryant + MNC.

Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Nicholas A. Bellinger]

On Mon, 2017-06-05 at 15:57 +, Bart Van Assche wrote:
> On Sat, 2017-06-03 at 22:10 +0000, Nicholas A. Bellinger wrote:
> > +static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
> > +  u64 *unpacked_lun)
> > +{
> > +   struct se_cmd *se_cmd;
> > +   unsigned long flags;
> > +   bool ret = false;
> > +
> > +   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
> > +   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
> > +   if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> > +   continue;
> > +
> > +   if (se_cmd->tag == tag) {
> > +   *unpacked_lun = se_cmd->orig_fe_lun;
> > +   ret = true;
> > +   break;
> > +   }
> > +   }
> > +   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
> > +
> > +   return ret;
> > +}
> > +
> >  /**
> >   * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
> >   * for TMR CDBs
> > @@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct 
> > se_session *se_sess,
> > core_tmr_release_req(se_cmd->se_tmr_req);
> > return ret;
> > }
> > +   /*
> > +* If this is ABORT_TASK with no explicit fabric provided LUN,
> > +* go ahead and search active session tags for a match to figure
> > +* out unpacked_lun for the original se_cmd.
> > +*/
> > +   if (tm_type == TMR_ABORT_TASK && (flags & 
> > TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
> > +   if (!target_lookup_lun_from_tag(se_sess, tag, _lun))
> > +   goto failure;
> > +   }
> >  
> > ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> > -   if (ret) {
> > -   /*
> > -* For callback during failure handling, push this work off
> > -* to process context with TMR_LUN_DOES_NOT_EXIST status.
> > -*/
> > -   INIT_WORK(_cmd->work, target_complete_tmr_failure);
> > -   schedule_work(_cmd->work);
> > -   return 0;
> > -   }
> > +   if (ret)
> > +   goto failure;
> > +
> > transport_generic_handle_tmr(se_cmd);
> > return 0;
> 
> Hello Nic,
> 
> So after LUN lookup has finished sess_cmd_lock lock is dropped, a context
> switch occurs due to the queue_work() call in transport_generic_handle_tmr()
> and next core_tmr_abort_task() reacquires that lock? Sorry but I'm afraid
> that if after that lock is dropped and before it is reacquired a command
> finishes and the initiator reuses the same tag for another command for the
> same LUN that this may result in the wrong command being aborted.

This is only getting a unpacked_lun to determine where the incoming
ABORT_TASK should perform a se_lun lookup and percpu ref upon.

The scenario you are talking about would require a tag be reused on the
same session for the same device between when the ABORT_TASK is
dispatched here, and actually processed.

Because qla2xxx doesn't reuse tags, it's not a problem since it's the
only consumer of TARGET_SCF_LOOKUP_LUN_FROM_TAG.

Since Himanshu and Quinn are OK it with, I'm OK with it.

If you have another driver that needs to use this logic where an
ABORT_TASK doesn't know the unpacked_lun to reference, and does reuse
tags then I'd consider a patch for that.

Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Nicholas A. Bellinger]

Hi Himanshu & Quinn,

On Wed, 2017-06-07 at 05:02 +, Madhani, Himanshu wrote:
> > On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <n...@linux-iscsi.org> 
> > wrote:
> > 
> > From: Nicholas Bellinger <n...@linux-iscsi.org>
> > 
> > Hi Himanshu + Quinn,
> > 
> > Here is a small series to introduce proper percpu se_lun->lun_ref
> > counting for TMR, and add common code in target_submit_tmr() to
> > do tag lookup for unpacked_lun in order to drop the original
> > driver specific lookup within __qlt_24xx_handle_abts().
> > 
> > It's rather straight-forward, so review and test as a v4.13 item.
> > 
> > Thanks!
> > 
> > Nicholas Bellinger (3):
> >  target: Add support for TMR percpu reference counting
> >  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
> >  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG
> > 
> > drivers/scsi/qla2xxx/qla_target.c  | 39 ++-
> > drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 ++-
> > drivers/target/target_core_device.c| 14 ++---
> > drivers/target/target_core_transport.c | 56 
> > --
> > include/target/target_core_base.h  |  3 +-
> > 5 files changed, 71 insertions(+), 45 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe target-devel" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Series looks good. 
> 
> Acked-by: Himanshu Madhani <himanshu.madh...@cavium.com>
> 

Thanks for the review! 

Re: [PATCH] iscsi-target: Fix delayed logout processing greater than SECONDS_FOR_LOGOUT_COMP

[Nicholas A. Bellinger]

Reviews pretty please..?

On Sat, 2017-06-03 at 21:32 +, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> This patch fixes a BUG() in iscsit_close_session() that could be
> triggered when iscsit_logout_post_handler() execution from within
> tx thread context was not run for more than SECONDS_FOR_LOGOUT_COMP
> (15 seconds), and the TCP connection didn't already close before
> then forcing tx thread context to automatically exit.
> 
> This would manifest itself during explicit logout as:
> 
> [33206.974254] 1 connection(s) still exist for iSCSI session to 
> iqn.1993-08.org.debian:01:3f5523242179
> [33206.980184] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 
> 2100.772 msecs
> [33209.078643] [ cut here ]
> [33209.078646] kernel BUG at drivers/target/iscsi/iscsi_target.c:4346!
> 
> Normally when explicit logout attempt fails, the tx thread context
> exits and iscsit_close_connection() from rx thread context does the
> extra cleanup once it detects conn->conn_logout_remove has not been
> cleared by the logout type specific post handlers.
> 
> To address this special case, if the logout post handler in tx thread
> context detects conn->tx_thread_active has already been cleared, simply
> return and exit in order for existing iscsit_close_connection()
> logic from rx thread context do failed logout cleanup.
> 
> Reported-by: Bart Van Assche <bart.vanass...@sandisk.com>
> Cc: Mike Christie <mchri...@redhat.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Sagi Grimberg <sa...@mellanox.com>
> Cc: sta...@vger.kernel.org # 3.14+
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/target/iscsi/iscsi_target.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c 
> b/drivers/target/iscsi/iscsi_target.c
> index 0d8f815..c025451 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -4423,8 +4423,11 @@ static void iscsit_logout_post_handler_closesession(
>* always sleep waiting for RX/TX thread shutdown to complete
>* within iscsit_close_connection().
>*/
> - if (!conn->conn_transport->rdma_shutdown)
> + if (!conn->conn_transport->rdma_shutdown) {
>   sleep = cmpxchg(>tx_thread_active, true, false);
> + if (!sleep)
> + return;
> + }
>  
>   atomic_set(>conn_logout_remove, 0);
>   complete(>conn_logout_comp);
> @@ -4440,8 +4443,11 @@ static void iscsit_logout_post_handler_samecid(
>  {
>   int sleep = 1;
>  
> - if (!conn->conn_transport->rdma_shutdown)
> + if (!conn->conn_transport->rdma_shutdown) {
>   sleep = cmpxchg(>tx_thread_active, true, false);
> + if (!sleep)
> + return;
> + }
>  
>   atomic_set(>conn_logout_remove, 0);
>   complete(>conn_logout_comp);

Re: [PATCH] target: Fix kref->refcount underflow in transport_cmd_finish_abort

[Nicholas A. Bellinger]

(Adding Quinn CC')

Reviews please..?

On Sat, 2017-06-03 at 21:09 +, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> This patch fixes a se_cmd->cmd_kref underflow during CMD_T_ABORTED
> when a fabric driver drops it's second reference from below the
> target_core_tmr.c based callers of transport_cmd_finish_abort().
> 
> Recently with the conversion of kref to refcount_t, this bug was
> manifesting itself as:
> 
> [705519.601034] refcount_t: underflow; use-after-free.
> [705519.604034] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 
> 20116.512 msecs
> [705539.719111] [ cut here ]
> [705539.719117] WARNING: CPU: 3 PID: 26510 at lib/refcount.c:184 
> refcount_sub_and_test+0x33/0x51
> 
> Since the original kref atomic_t based kref_put() didn't check for
> underflow and only invoked the final callback when zero was reached,
> this bug did not manifest in practice since all se_cmd memory is
> using preallocated tags.
> 
> To address this, go ahead and propigate the existing return from
> transport_put_cmd() up via transport_cmd_finish_abort(), and
> change transport_cmd_finish_abort() + core_tmr_handle_tas_abort()
> callers to only do their local target_put_sess_cmd() if necessary.
> 
> Reported-by: Bart Van Assche <bart.vanass...@sandisk.com>
> Cc: Mike Christie <mchri...@redhat.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Himanshu Madhani <himanshu.madh...@qlogic.com>
> Cc: Sagi Grimberg <sa...@mellanox.com>
> Cc: sta...@vger.kernel.org # 3.14+
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/target/target_core_internal.h  |  2 +-
>  drivers/target/target_core_tmr.c   | 16 
>  drivers/target/target_core_transport.c |  9 ++---
>  3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/target/target_core_internal.h 
> b/drivers/target/target_core_internal.h
> index 9ab7090..0912de7 100644
> --- a/drivers/target/target_core_internal.h
> +++ b/drivers/target/target_core_internal.h
> @@ -136,7 +136,7 @@ struct se_node_acl 
> *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
>  void release_se_kmem_caches(void);
>  u32  scsi_get_new_index(scsi_index_t);
>  void transport_subsystem_check_init(void);
> -void transport_cmd_finish_abort(struct se_cmd *, int);
> +int  transport_cmd_finish_abort(struct se_cmd *, int);
>  unsigned char *transport_dump_cmd_direction(struct se_cmd *);
>  void transport_dump_dev_state(struct se_device *, char *, int *);
>  void transport_dump_dev_info(struct se_device *, struct se_lun *,
> diff --git a/drivers/target/target_core_tmr.c 
> b/drivers/target/target_core_tmr.c
> index dce1e1b..13f47bf 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -75,7 +75,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
>   kfree(tmr);
>  }
>  
> -static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
> +static int core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
>  {
>   unsigned long flags;
>   bool remove = true, send_tas;
> @@ -91,7 +91,7 @@ static void core_tmr_handle_tas_abort(struct se_cmd *cmd, 
> int tas)
>   transport_send_task_abort(cmd);
>   }
>  
> - transport_cmd_finish_abort(cmd, remove);
> + return transport_cmd_finish_abort(cmd, remove);
>  }
>  
>  static int target_check_cdb_and_preempt(struct list_head *list,
> @@ -184,8 +184,8 @@ void core_tmr_abort_task(
>   cancel_work_sync(_cmd->work);
>   transport_wait_for_tasks(se_cmd);
>  
> - transport_cmd_finish_abort(se_cmd, true);
> - target_put_sess_cmd(se_cmd);
> + if (!transport_cmd_finish_abort(se_cmd, true))
> + target_put_sess_cmd(se_cmd);
>  
>   printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
>   " ref_tag: %llu\n", ref_tag);
> @@ -281,8 +281,8 @@ static void core_tmr_drain_tmr_list(
>   cancel_work_sync(>work);
>   transport_wait_for_tasks(cmd);
>  
> - transport_cmd_finish_abort(cmd, 1);
> - target_put_sess_cmd(cmd);
> + if (!transport_cmd_finish_abort(cmd, 1))
> + target_put_sess_cmd(cmd);
>   }
>  }
>  
> @@ -380,8 +380,8 @@ static void core_tmr_drain_state_list(
>   cancel_work_sync(>work);
>   transport_wait_for_tasks(cmd);
>  
> - core_tmr_handle_tas_abort(cmd, tas);
> - targe

[PATCH] configfs: Fix race between create_link and configfs_rmdir

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch closes a long standing race in configfs between
the creation of a new symlink in create_link(), while the
symlink target's config_item is being concurrently removed
via configfs_rmdir().

This can happen because the symlink target's reference
is obtained by config_item_get() in create_link() before
the CONFIGFS_USET_DROPPING bit set by configfs_detach_prep()
during configfs_rmdir() shutdown is actually checked..

This originally manifested itself on ppc64 on v4.8.y under
heavy load using ibmvscsi target ports with Novalink API:

[ 7877.289863] rpadlpar_io: slot U8247.22L.212A91A-V1-C8 added
[ 7879.893760] [ cut here ]
[ 7879.893768] WARNING: CPU: 15 PID: 17585 at ./include/linux/kref.h:46 
config_item_get+0x7c/0x90 [configfs]
[ 7879.893811] CPU: 15 PID: 17585 Comm: targetcli Tainted: G   O 
4.8.17-customv2.22 #12
[ 7879.893812] task: c0018a0d3400 task.stack: c001f3b4
[ 7879.893813] NIP: d2c664ec LR: d2c60980 CTR: c0b70870
[ 7879.893814] REGS: c001f3b43810 TRAP: 0700   Tainted: G O 
(4.8.17-customv2.22)
[ 7879.893815] MSR: 80029033 <SF,EE,ME,IR,DR,RI,LE>  CR: 2842  XER: 

[ 7879.893820] CFAR: d2c664bc SOFTE: 1
GPR00: d2c60980 c001f3b43a90 d2c70908 
c000fbc06820
GPR04: c001ef1bd900 0004 0001 

GPR08:  0001 d2c69560 
d2c66d80
GPR12: c0b70870 ce798700 c001f3b43ca0 
c001d4949d40
GPR16: c0014637e1c0   
c000f2392940
GPR20: c001f3b43b98 0041 0060 

GPR24: f000  d2c60be0 
c001f1dac490
GPR28: 0004  c001ef1bd900 
c000f2392940
[ 7879.893839] NIP [d2c664ec] config_item_get+0x7c/0x90 [configfs]
[ 7879.893841] LR [d2c60980] check_perm+0x80/0x2e0 [configfs]
[ 7879.893842] Call Trace:
[ 7879.893844] [c001f3b43ac0] [d2c60980] check_perm+0x80/0x2e0 
[configfs]
[ 7879.893847] [c001f3b43b10] [c0329770] do_dentry_open+0x2c0/0x460
[ 7879.893849] [c001f3b43b70] [c0344480] path_openat+0x210/0x1490
[ 7879.893851] [c001f3b43c80] [c034708c] do_filp_open+0xfc/0x170
[ 7879.893853] [c001f3b43db0] [c032b5bc] do_sys_open+0x1cc/0x390
[ 7879.893856] [c001f3b43e30] [c0009584] system_call+0x38/0xec
[ 7879.893856] Instruction dump:
[ 7879.893858] 409d0014 38210030 e8010010 7c0803a6 4e800020 3d22 e94981e0 
892a
[ 7879.893861] 2f89 409effe0 3921 992a <0fe0> 4bd0 6000 
6000
[ 7879.893866] ---[ end trace 14078f0b3b5ad0aa ]---

To close this race, go ahead and obtain the symlink's target
config_item reference only after the existing CONFIGFS_USET_DROPPING
check succeeds.

This way, if configfs_rmdir() wins create_link() will return -ENONET,
and if create_link() wins configfs_rmdir() will return -EBUSY.

Reported-by: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
Tested-by: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
Cc: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Joel Becker <jl...@evilplan.org>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 fs/configfs/symlink.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index a6ab012..c8aabba 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -83,14 +83,13 @@ static int create_link(struct config_item *parent_item,
ret = -ENOMEM;
sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
if (sl) {
-   sl->sl_target = config_item_get(item);
spin_lock(_dirent_lock);
if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
spin_unlock(_dirent_lock);
-   config_item_put(item);
kfree(sl);
return -ENOENT;
}
+   sl->sl_target = config_item_get(item);
list_add(>sl_list, _sd->s_links);
spin_unlock(_dirent_lock);
ret = configfs_create_link(sl, parent_item->ci_dentry,
-- 
1.9.1

[PATCH] iscsi-target: Reject immediate data underflow larger than SCSI transfer length

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

When iscsi WRITE underflow occurs there are two different scenarios
that can happen.

Normally in practice, when an EDTL vs. SCSI CDB TRANSFER LENGTH
underflow is detected, the iscsi immediate data payload is the
smaller SCSI CDB TRANSFER LENGTH.

That is, when a host fabric LLD is using a fixed size EDTL for
a specific control CDB, the SCSI CDB TRANSFER LENGTH and actual
SCSI payload ends up being smaller than EDTL.  In iscsi, this
means the received iscsi immediate data payload matches the
smaller SCSI CDB TRANSFER LENGTH, because there is no more
SCSI payload to accept beyond SCSI CDB TRANSFER LENGTH.

However, it's possible for a malicous host to send a WRITE
underflow where EDTL is larger than SCSI CDB TRANSFER LENGTH,
but incoming iscsi immediate data actually matches EDTL.

In the wild, we've never had a iscsi host environment actually
try to do this.

For this special case, it's wrong to truncate part of the
control CDB payload and continue to process the command during
underflow when immediate data payload received was larger than
SCSI CDB TRANSFER LENGTH, so go ahead and reject and drop the
bogus payload as a defensive action.

Note this potential bug was originally relaxed by the following
for allowing WRITE underflow in MSFT FCP host environments:

   commit c72c5250224d475614a00c1d7e54a67f77cd3410
   Author: Roland Dreier <rol...@purestorage.com>
   Date:   Wed Jul 22 15:08:18 2015 -0700

  target: allow underflow/overflow for PR OUT etc. commands

Cc: Roland Dreier <rol...@purestorage.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.de>
Cc: Martin K. Petersen <martin.peter...@oracle.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index c025451..3fdca2c 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1279,6 +1279,18 @@ int iscsit_process_scsi_cmd(struct iscsi_conn *conn, 
struct iscsi_cmd *cmd,
 */
if (dump_payload)
goto after_immediate_data;
+   /*
+* Check for underflow case where both EDTL and immediate data payload
+* exceeds what is presented by CDB's TRANSFER LENGTH, and what has
+* already been set in target_cmd_size_check() as se_cmd->data_length.
+*
+* For this special case, fail the command and dump the immediate data
+* payload.
+*/
+   if (cmd->first_burst_len > cmd->se_cmd.data_length) {
+   cmd->sense_reason = TCM_INVALID_CDB_FIELD;
+   goto after_immediate_data;
+   }
 
immed_ret = iscsit_handle_immediate_data(cmd, hdr,
cmd->first_burst_len);
-- 
1.9.1

[PATCH 1/3] target: Add support for TMR percpu reference counting

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch introduces TMR percpu reference counting using
se_lun->lun_ref in transport_lookup_tmr_lun(), following
how existing non TMR per se_lun reference counting works
within transport_lookup_cmd_lun().

It also adds explicit transport_lun_remove_cmd() calls to
drop the reference in the three tmr related locations that
invoke transport_cmd_check_stop_to_fabric();

   - target_tmr_work() during normal ->queue_tm_rsp()
   - target_complete_tmr_failure() during error ->queue_tm_rsp()
   - transport_generic_handle_tmr() during early failure

Also, note the exception paths in transport_generic_free_cmd()
and transport_cmd_finish_abort() already check SCF_SE_LUN_CMD,
and will invoke transport_lun_remove_cmd() when necessary.

Cc: Himanshu Madhani <himanshu.madh...@cavium.com>
Cc: Quinn Tran <quinn.t...@cavium.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_device.c| 14 ++
 drivers/target/target_core_transport.c |  3 +++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 8f0e0e3..11c80c4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -168,11 +168,20 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 
unpacked_lun)
rcu_read_lock();
deve = target_nacl_find_deve(nacl, unpacked_lun);
if (deve) {
-   se_cmd->se_lun = rcu_dereference(deve->se_lun);
se_lun = rcu_dereference(deve->se_lun);
+
+   if (!percpu_ref_tryget_live(_lun->lun_ref)) {
+   se_lun = NULL;
+   goto out_unlock;
+   }
+
+   se_cmd->se_lun = rcu_dereference(deve->se_lun);
se_cmd->pr_res_key = deve->pr_res_key;
se_cmd->orig_fe_lun = unpacked_lun;
+   se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
+   se_cmd->lun_ref_active = true;
}
+out_unlock:
rcu_read_unlock();
 
if (!se_lun) {
@@ -182,9 +191,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 
unpacked_lun)
unpacked_lun);
return -ENODEV;
}
-   /*
-* XXX: Add percpu se_lun->lun_ref reference count for TMR
-*/
se_cmd->se_dev = rcu_dereference_raw(se_lun->lun_se_dev);
se_tmr->tmr_dev = rcu_dereference_raw(se_lun->lun_se_dev);
 
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index f16a789..83bfc97 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1588,6 +1588,7 @@ static void target_complete_tmr_failure(struct 
work_struct *work)
se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
se_cmd->se_tfo->queue_tm_rsp(se_cmd);
 
+   transport_lun_remove_cmd(se_cmd);
transport_cmd_check_stop_to_fabric(se_cmd);
 }
 
@@ -3199,6 +3200,7 @@ static void target_tmr_work(struct work_struct *work)
cmd->se_tfo->queue_tm_rsp(cmd);
 
 check_stop:
+   transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
 }
 
@@ -3221,6 +3223,7 @@ int transport_generic_handle_tmr(
pr_warn_ratelimited("handle_tmr caught CMD_T_ABORTED TMR %d"
"ref_tag: %llu tag: %llu\n", cmd->se_tmr_req->function,
cmd->se_tmr_req->ref_task_tag, cmd->tag);
+   transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return 0;
}
-- 
1.9.1

[PATCH 3/3] qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Following Himanshu's earlier patch to drop the redundant tag
lookup within __qlt_24xx_handle_abts(), go ahead and drop this
now QLA_TGT_ABTS can use TARGET_SCF_LOOKUP_LUN_FROM_TAG and
have target_submit_tmr() do this from common code.

Cc: Himanshu Madhani <himanshu.madh...@cavium.com>
Cc: Quinn Tran <quinn.t...@cavium.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/qla_target.c  | 39 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 +++-
 2 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 0e03ca2..401e245 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1847,38 +1847,13 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host 
*vha,
struct abts_recv_from_24xx *abts, struct fc_port *sess)
 {
struct qla_hw_data *ha = vha->hw;
-   struct se_session *se_sess = sess->se_sess;
struct qla_tgt_mgmt_cmd *mcmd;
-   struct se_cmd *se_cmd;
-   u32 lun = 0;
int rc;
-   bool found_lun = false;
-   unsigned long flags;
-
-   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
-   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
-   struct qla_tgt_cmd *cmd =
-   container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
-   if (se_cmd->tag == abts->exchange_addr_to_abort) {
-   lun = cmd->unpacked_lun;
-   found_lun = true;
-   break;
-   }
-   }
-   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
 
-   /* cmd not in LIO lists, look in qla list */
-   if (!found_lun) {
-   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
-   /* send TASK_ABORT response immediately */
-   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
-   return 0;
-   } else {
-   ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
-   "unable to find cmd in driver or LIO for tag 
0x%x\n",
-   abts->exchange_addr_to_abort);
-   return -ENOENT;
-   }
+   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
+   /* send TASK_ABORT response immediately */
+   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
+   return 0;
}
 
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
@@ -1899,7 +1874,11 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host 
*vha,
mcmd->reset_count = vha->hw->chip_reset;
mcmd->tmr_func = QLA_TGT_ABTS;
 
-   rc = ha->tgt.tgt_ops->handle_tmr(mcmd, lun, mcmd->tmr_func,
+   /*
+* LUN is looked up by target-core internally based on the passed
+* abts->exchange_addr_to_abort tag.
+*/
+   rc = ha->tgt.tgt_ops->handle_tmr(mcmd, 0, mcmd->tmr_func,
abts->exchange_addr_to_abort);
if (rc != 0) {
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7443e4e..75aeb9f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -601,11 +601,13 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd 
*mcmd, uint32_t lun,
struct fc_port *sess = mcmd->sess;
struct se_cmd *se_cmd = >se_cmd;
int transl_tmr_func = 0;
+   int flags = TARGET_SCF_ACK_KREF;
 
switch (tmr_func) {
case QLA_TGT_ABTS:
pr_debug("%ld: ABTS received\n", sess->vha->host_no);
transl_tmr_func = TMR_ABORT_TASK;
+   flags |= TARGET_SCF_LOOKUP_LUN_FROM_TAG;
break;
case QLA_TGT_2G_ABORT_TASK:
pr_debug("%ld: 2G Abort Task received\n", sess->vha->host_no);
@@ -638,7 +640,7 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd 
*mcmd, uint32_t lun,
}
 
return target_submit_tmr(se_cmd, sess->se_sess, NULL, lun, mcmd,
-   transl_tmr_func, GFP_ATOMIC, tag, TARGET_SCF_ACK_KREF);
+   transl_tmr_func, GFP_ATOMIC, tag, flags);
 }
 
 static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
-- 
1.9.1

[PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Hi Himanshu + Quinn,

Here is a small series to introduce proper percpu se_lun->lun_ref
counting for TMR, and add common code in target_submit_tmr() to
do tag lookup for unpacked_lun in order to drop the original
driver specific lookup within __qlt_24xx_handle_abts().

It's rather straight-forward, so review and test as a v4.13 item.

Thanks!

Nicholas Bellinger (3):
  target: Add support for TMR percpu reference counting
  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

 drivers/scsi/qla2xxx/qla_target.c  | 39 ++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 ++-
 drivers/target/target_core_device.c| 14 ++---
 drivers/target/target_core_transport.c | 56 --
 include/target/target_core_base.h  |  3 +-
 5 files changed, 71 insertions(+), 45 deletions(-)

-- 
1.9.1

[PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch introduces support in target_submit_tmr() for locating a
unpacked_lun from an existing se_cmd->tag during ABORT_TASK.

When TARGET_SCF_LOOKUP_LUN_FROM_TAG is set, target_submit_tmr()
will do the extra lookup via target_lookup_lun_from_tag() and
subsequently invoke transport_lookup_tmr_lun() so a proper
percpu se_lun->lun_ref is taken before workqueue dispatch into
se_device->tmr_wq happens.

Aside from the extra target_lookup_lun_from_tag(), the existing
code-path remains unchanged.

Cc: Himanshu Madhani <himanshu.madh...@cavium.com>
Cc: Quinn Tran <quinn.t...@cavium.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 53 --
 include/target/target_core_base.h  |  3 +-
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 83bfc97..dbb8101 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1592,6 +1592,29 @@ static void target_complete_tmr_failure(struct 
work_struct *work)
transport_cmd_check_stop_to_fabric(se_cmd);
 }
 
+static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
+  u64 *unpacked_lun)
+{
+   struct se_cmd *se_cmd;
+   unsigned long flags;
+   bool ret = false;
+
+   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
+   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
+   if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+   continue;
+
+   if (se_cmd->tag == tag) {
+   *unpacked_lun = se_cmd->orig_fe_lun;
+   ret = true;
+   break;
+   }
+   }
+   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
+
+   return ret;
+}
+
 /**
  * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
  * for TMR CDBs
@@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct 
se_session *se_sess,
core_tmr_release_req(se_cmd->se_tmr_req);
return ret;
}
+   /*
+* If this is ABORT_TASK with no explicit fabric provided LUN,
+* go ahead and search active session tags for a match to figure
+* out unpacked_lun for the original se_cmd.
+*/
+   if (tm_type == TMR_ABORT_TASK && (flags & 
TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
+   if (!target_lookup_lun_from_tag(se_sess, tag, _lun))
+   goto failure;
+   }
 
ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
-   if (ret) {
-   /*
-* For callback during failure handling, push this work off
-* to process context with TMR_LUN_DOES_NOT_EXIST status.
-*/
-   INIT_WORK(_cmd->work, target_complete_tmr_failure);
-   schedule_work(_cmd->work);
-   return 0;
-   }
+   if (ret)
+   goto failure;
+
transport_generic_handle_tmr(se_cmd);
return 0;
+
+   /*
+* For callback during failure handling, push this work off
+* to process context with TMR_LUN_DOES_NOT_EXIST status.
+*/
+failure:
+   INIT_WORK(_cmd->work, target_complete_tmr_failure);
+   schedule_work(_cmd->work);
+   return 0;
 }
 EXPORT_SYMBOL(target_submit_tmr);
 
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index db2c7b3..a3af69f 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -188,7 +188,8 @@ enum target_sc_flags_table {
TARGET_SCF_BIDI_OP  = 0x01,
TARGET_SCF_ACK_KREF = 0x02,
TARGET_SCF_UNKNOWN_SIZE = 0x04,
-   TARGET_SCF_USE_CPUID= 0x08,
+   TARGET_SCF_USE_CPUID= 0x08,
+   TARGET_SCF_LOOKUP_LUN_FROM_TAG  = 0x10,
 };
 
 /* fabric independent task management function values */
-- 
1.9.1

[PATCH] iscsi-target: Fix delayed logout processing greater than SECONDS_FOR_LOGOUT_COMP

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a BUG() in iscsit_close_session() that could be
triggered when iscsit_logout_post_handler() execution from within
tx thread context was not run for more than SECONDS_FOR_LOGOUT_COMP
(15 seconds), and the TCP connection didn't already close before
then forcing tx thread context to automatically exit.

This would manifest itself during explicit logout as:

[33206.974254] 1 connection(s) still exist for iSCSI session to 
iqn.1993-08.org.debian:01:3f5523242179
[33206.980184] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 
2100.772 msecs
[33209.078643] [ cut here ]
[33209.078646] kernel BUG at drivers/target/iscsi/iscsi_target.c:4346!

Normally when explicit logout attempt fails, the tx thread context
exits and iscsit_close_connection() from rx thread context does the
extra cleanup once it detects conn->conn_logout_remove has not been
cleared by the logout type specific post handlers.

To address this special case, if the logout post handler in tx thread
context detects conn->tx_thread_active has already been cleared, simply
return and exit in order for existing iscsit_close_connection()
logic from rx thread context do failed logout cleanup.

Reported-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.de>
Cc: Sagi Grimberg <sa...@mellanox.com>
Cc: sta...@vger.kernel.org # 3.14+
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 0d8f815..c025451 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4423,8 +4423,11 @@ static void iscsit_logout_post_handler_closesession(
 * always sleep waiting for RX/TX thread shutdown to complete
 * within iscsit_close_connection().
 */
-   if (!conn->conn_transport->rdma_shutdown)
+   if (!conn->conn_transport->rdma_shutdown) {
sleep = cmpxchg(>tx_thread_active, true, false);
+   if (!sleep)
+   return;
+   }
 
atomic_set(>conn_logout_remove, 0);
complete(>conn_logout_comp);
@@ -4440,8 +4443,11 @@ static void iscsit_logout_post_handler_samecid(
 {
int sleep = 1;
 
-   if (!conn->conn_transport->rdma_shutdown)
+   if (!conn->conn_transport->rdma_shutdown) {
sleep = cmpxchg(>tx_thread_active, true, false);
+   if (!sleep)
+   return;
+   }
 
atomic_set(>conn_logout_remove, 0);
complete(>conn_logout_comp);
-- 
1.9.1

[PATCH] target: Fix kref->refcount underflow in transport_cmd_finish_abort

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a se_cmd->cmd_kref underflow during CMD_T_ABORTED
when a fabric driver drops it's second reference from below the
target_core_tmr.c based callers of transport_cmd_finish_abort().

Recently with the conversion of kref to refcount_t, this bug was
manifesting itself as:

[705519.601034] refcount_t: underflow; use-after-free.
[705519.604034] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 
20116.512 msecs
[705539.719111] [ cut here ]
[705539.719117] WARNING: CPU: 3 PID: 26510 at lib/refcount.c:184 
refcount_sub_and_test+0x33/0x51

Since the original kref atomic_t based kref_put() didn't check for
underflow and only invoked the final callback when zero was reached,
this bug did not manifest in practice since all se_cmd memory is
using preallocated tags.

To address this, go ahead and propigate the existing return from
transport_put_cmd() up via transport_cmd_finish_abort(), and
change transport_cmd_finish_abort() + core_tmr_handle_tas_abort()
callers to only do their local target_put_sess_cmd() if necessary.

Reported-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.de>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Himanshu Madhani <himanshu.madh...@qlogic.com>
Cc: Sagi Grimberg <sa...@mellanox.com>
Cc: sta...@vger.kernel.org # 3.14+
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_internal.h  |  2 +-
 drivers/target/target_core_tmr.c   | 16 
 drivers/target/target_core_transport.c |  9 ++---
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_internal.h 
b/drivers/target/target_core_internal.h
index 9ab7090..0912de7 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -136,7 +136,7 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(struct 
se_portal_group *tpg,
 void   release_se_kmem_caches(void);
 u32scsi_get_new_index(scsi_index_t);
 void   transport_subsystem_check_init(void);
-void   transport_cmd_finish_abort(struct se_cmd *, int);
+inttransport_cmd_finish_abort(struct se_cmd *, int);
 unsigned char *transport_dump_cmd_direction(struct se_cmd *);
 void   transport_dump_dev_state(struct se_device *, char *, int *);
 void   transport_dump_dev_info(struct se_device *, struct se_lun *,
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index dce1e1b..13f47bf 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -75,7 +75,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
kfree(tmr);
 }
 
-static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
+static int core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
 {
unsigned long flags;
bool remove = true, send_tas;
@@ -91,7 +91,7 @@ static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int 
tas)
transport_send_task_abort(cmd);
}
 
-   transport_cmd_finish_abort(cmd, remove);
+   return transport_cmd_finish_abort(cmd, remove);
 }
 
 static int target_check_cdb_and_preempt(struct list_head *list,
@@ -184,8 +184,8 @@ void core_tmr_abort_task(
cancel_work_sync(_cmd->work);
transport_wait_for_tasks(se_cmd);
 
-   transport_cmd_finish_abort(se_cmd, true);
-   target_put_sess_cmd(se_cmd);
+   if (!transport_cmd_finish_abort(se_cmd, true))
+   target_put_sess_cmd(se_cmd);
 
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
" ref_tag: %llu\n", ref_tag);
@@ -281,8 +281,8 @@ static void core_tmr_drain_tmr_list(
cancel_work_sync(>work);
transport_wait_for_tasks(cmd);
 
-   transport_cmd_finish_abort(cmd, 1);
-   target_put_sess_cmd(cmd);
+   if (!transport_cmd_finish_abort(cmd, 1))
+   target_put_sess_cmd(cmd);
}
 }
 
@@ -380,8 +380,8 @@ static void core_tmr_drain_state_list(
cancel_work_sync(>work);
transport_wait_for_tasks(cmd);
 
-   core_tmr_handle_tas_abort(cmd, tas);
-   target_put_sess_cmd(cmd);
+   if (!core_tmr_handle_tas_abort(cmd, tas))
+   target_put_sess_cmd(cmd);
}
 }
 
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 6025935..f1b3a46 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -651,9 +651,10 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
percpu_ref_put(>lun_ref);
 }
 
-void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
+int transport_cmd_finish_

Re: [PATCH v2 0/5] TCMU Enable Reconfiguration Patches

[Nicholas A. Bellinger]

Hi Bryant & Co,

On Tue, 2017-05-30 at 13:31 -0500, Bryant G. Ly wrote:
> This patch consists of adding a netlink to allow for reconfiguration
> of a device in tcmu.
> 
> It also changes and adds some attributes that are reconfigurable:
> write_cache, device size, and device path.
> 
> Bryant G. Ly (5):
>   tcmu: Support emulate_write_cache
>   tcmu: Add netlink for device reconfiguration
>   tcmu: Make dev_size configurable via userspace
>   tcmu: Make dev_config configurable
>   tcmu: Add Type of reconfig into netlink
> 
>  drivers/target/target_core_user.c | 159 
> --
>  include/uapi/linux/target_core_user.h |   2 +
>  2 files changed, 155 insertions(+), 6 deletions(-)
> 

Are these ready for merge..?

MNC..?

Re: [PATCH] iscsi: Fix a sleep-in-atomic bug

[Nicholas A. Bellinger]

On Fri, 2017-06-02 at 09:13 +0800, Jia-Ju Bai wrote:
> On 06/01/2017 02:21 PM, Nicholas A. Bellinger wrote:
> > Hi Jia-Ju,
> >
> > On Wed, 2017-05-31 at 11:26 +0800, Jia-Ju Bai wrote:
> >> The driver may sleep under a spin lock, and the function call path is:
> >> iscsit_tpg_enable_portal_group (acquire the lock by spin_lock)
> >>iscsi_update_param_value
> >>  kstrdup(GFP_KERNEL) -->  may sleep
> >>
> >> To fix it, the "GFP_KERNEL" is replaced with "GFP_ATOMIC".
> >>
> >> Signed-off-by: Jia-Ju Bai<baijiaju1...@163.com>
> >> ---
> >>   drivers/target/iscsi/iscsi_target_parameters.c |2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > Btw, the use of tpg->tpg_state_lock in iscsit_tpg_enable_portal_group()
> > while checking existing state and calling iscsi_update_param_value() is
> > not necessary, since lio_target_tpg_enable_store() is already holding
> > iscsit_get_tpg() ->  tpg->tpg_access_lock.
> >
> > How about the following instead to only take tpg->tpg_state_lock when
> > updating tpg->tpg_state instead..?
> >
> > diff --git a/drivers/target/iscsi/iscsi_target_tpg.c 
> > b/drivers/target/iscsi/iscsi_target_tpg.c
> > index 2e7e08d..abaabba 100644
> > --- a/drivers/target/iscsi/iscsi_target_tpg.c
> > +++ b/drivers/target/iscsi/iscsi_target_tpg.c
> > @@ -311,11 +311,9 @@ int iscsit_tpg_enable_portal_group(struct 
> > iscsi_portal_group *tpg)
> >  struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
> >  int ret;
> >
> > -   spin_lock(>tpg_state_lock);
> >  if (tpg->tpg_state == TPG_STATE_ACTIVE) {
> >  pr_err("iSCSI target portal group: %hu is already"
> >  " active, ignoring request.\n", tpg->tpgt);
> > -   spin_unlock(>tpg_state_lock);
> >  return -EINVAL;
> >  }
> >  /*
> > @@ -324,10 +322,8 @@ int iscsit_tpg_enable_portal_group(struct 
> > iscsi_portal_group *tpg)
> >   * is enforced (as per default), and remove the NONE option.
> >   */
> >  param = iscsi_find_param_from_key(AUTHMETHOD, tpg->param_list);
> > -   if (!param) {
> > -   spin_unlock(>tpg_state_lock);
> > +   if (!param)
> >  return -EINVAL;
> > -   }
> >
> >  if (tpg->tpg_attrib.authentication) {
> >  if (!strcmp(param->value, NONE)) {
> > @@ -341,6 +337,7 @@ int iscsit_tpg_enable_portal_group(struct 
> > iscsi_portal_group *tpg)
> >  goto err;
> >  }
> >
> > +   spin_lock(>tpg_state_lock);
> >  tpg->tpg_state = TPG_STATE_ACTIVE;
> >  spin_unlock(>tpg_state_lock);
> >
> > @@ -353,7 +350,6 @@ int iscsit_tpg_enable_portal_group(struct 
> > iscsi_portal_group *tpg)
> >  return 0;
> >
> >   err:
> > -   spin_unlock(>tpg_state_lock);
> >  return ret;
> >   }
> >
> I think it is fine to me.
> 
> Thanks,
> Jia-Ju Bai

Applied with your Reported-by and Reviewed-by.

Thanks!

[PATCH-v2] target: Avoid target_shutdown_sessions loop during queue_depth change

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

When target_shutdown_sessions() is invoked to shutdown all active
sessions associated with a se_node_acl when se_node_acl->queue_depth
is changed via core_tpg_set_initiator_node_queue_depth(), it's
possible that new connections reconnect immediately after explicit
shutdown occurs via target_shutdown_sessions().

Which means it's possible for the newly reconnected session with
the proper queue_depth can be shutdown multiple times when
target_shutdown_sessions() loops to drain all active sessions
for all cases.

This was regression was introduced by:

  commit bc6e6bb470eda42f44bcac96c261cff1216577b3
  Author: Christoph Hellwig <h...@lst.de>
  Date:   Mon May 2 15:45:19 2016 +0200

  target: consolidate and fix session shutdown

To avoid this case, instead change target_shutdown_sessions() to
pass 'do_restart' and avoid the looping drain of sessions when
invoked via core_tpg_set_initiator_node_queue_depth(), but still
loop during normal se_node_acl delete until all associated
sessions have been shutdown.

(v2 - go back to the original version instead of a local list,
 in order to protect list_del_init(>sess_acl_list) from
 transport_deregister_session_configfs.
 Also use safe list walking in target_shutdown_sessions - nab)

Cc: Christoph Hellwig <h...@lst.de>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_tpg.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 3691373..1b2b60e 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -336,14 +336,14 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(
return acl;
 }
 
-static void target_shutdown_sessions(struct se_node_acl *acl)
+static void target_shutdown_sessions(struct se_node_acl *acl, bool do_restart)
 {
-   struct se_session *sess;
+   struct se_session *sess, *sess_tmp;
unsigned long flags;
 
 restart:
spin_lock_irqsave(>nacl_sess_lock, flags);
-   list_for_each_entry(sess, >acl_sess_list, sess_acl_list) {
+   list_for_each_entry_safe(sess, sess_tmp, >acl_sess_list, 
sess_acl_list) {
if (sess->sess_tearing_down)
continue;
 
@@ -352,7 +352,11 @@ static void target_shutdown_sessions(struct se_node_acl 
*acl)
 
if (acl->se_tpg->se_tpg_tfo->close_session)
acl->se_tpg->se_tpg_tfo->close_session(sess);
-   goto restart;
+
+   if (do_restart)
+   goto restart;
+
+   spin_lock_irqsave(>nacl_sess_lock, flags);
}
spin_unlock_irqrestore(>nacl_sess_lock, flags);
 }
@@ -367,7 +371,7 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl 
*acl)
list_del(>acl_list);
mutex_unlock(>acl_node_mutex);
 
-   target_shutdown_sessions(acl);
+   target_shutdown_sessions(acl, true);
 
target_put_nacl(acl);
/*
@@ -414,7 +418,7 @@ int core_tpg_set_initiator_node_queue_depth(
/*
 * Shutdown all pending sessions to force session reinstatement.
 */
-   target_shutdown_sessions(acl);
+   target_shutdown_sessions(acl, false);
 
pr_debug("Successfully changed queue depth to: %d for Initiator"
" Node: %s on %s Target Portal Group: %u\n", acl->queue_depth,
-- 
1.9.1

[PATCH 1/2] target/configfs: Kill se_device->dev_link_magic

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Instead of using a hardcoded magic value in se_device when verifying
a target config_item symlink source during target_fabric_port_link(),
go ahead and use target_core_dev_item_ops directly instead.

Cc: Christoph Hellwig <h...@lst.de>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_configfs.c|  6 +-
 drivers/target/target_core_device.c  |  1 -
 drivers/target/target_core_fabric_configfs.c | 12 +++-
 include/target/target_core_base.h|  2 --
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 0326607..9b8abd5 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -2236,7 +2236,11 @@ static void target_core_dev_release(struct config_item 
*item)
target_free_device(dev);
 }
 
-static struct configfs_item_operations target_core_dev_item_ops = {
+/*
+ * Used in target_core_fabric_configfs.c to verify valid se_device symlink
+ * within target_fabric_port_link()
+ */
+struct configfs_item_operations target_core_dev_item_ops = {
.release= target_core_dev_release,
 };
 
diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index a5762e6..e4771ce 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -756,7 +756,6 @@ struct se_device *target_alloc_device(struct se_hba *hba, 
const char *name)
if (!dev)
return NULL;
 
-   dev->dev_link_magic = SE_DEV_LINK_MAGIC;
dev->se_hba = hba;
dev->transport = hba->backend->ops;
dev->prot_length = sizeof(struct t10_pi_tuple);
diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index d1e6cab..2cbaecd 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -620,6 +620,8 @@ static ssize_t target_fabric_port_alua_tg_pt_write_md_store(
NULL,
 };
 
+extern struct configfs_item_operations target_core_dev_item_ops;
+
 static int target_fabric_port_link(
struct config_item *lun_ci,
struct config_item *se_dev_ci)
@@ -628,16 +630,16 @@ static int target_fabric_port_link(
struct se_lun *lun = container_of(to_config_group(lun_ci),
struct se_lun, lun_group);
struct se_portal_group *se_tpg;
-   struct se_device *dev =
-   container_of(to_config_group(se_dev_ci), struct se_device, 
dev_group);
+   struct se_device *dev;
struct target_fabric_configfs *tf;
int ret;
 
-   if (dev->dev_link_magic != SE_DEV_LINK_MAGIC) {
-   pr_err("Bad dev->dev_link_magic, not a valid se_dev_ci pointer:"
-   " %p to struct se_device: %p\n", se_dev_ci, dev);
+   if (!se_dev_ci->ci_type ||
+   se_dev_ci->ci_type->ct_item_ops != _core_dev_item_ops) {
+   pr_err("Bad se_dev_ci, not a valid se_dev_ci pointer: %p\n", 
se_dev_ci);
return -EFAULT;
}
+   dev = container_of(to_config_group(se_dev_ci), struct se_device, 
dev_group);
 
if (!(dev->dev_flags & DF_CONFIGURED)) {
pr_err("se_device not configured yet, cannot port link\n");
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index 0c1dce2..c3c14d0 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -746,8 +746,6 @@ struct se_dev_stat_grps {
 };
 
 struct se_device {
-#define SE_DEV_LINK_MAGIC  0xfeeddeef
-   u32 dev_link_magic;
/* RELATIVE TARGET PORT IDENTIFER Counter */
u16 dev_rpti_counter;
/* Used for SAM Task Attribute ordering */
-- 
1.9.1

[PATCH 2/2] target/configfs: Kill se_lun->lun_link_magic

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Instead of using a hardcoded magic value in se_lun when verifying
a target config_item symlink source during target_fabric_mappedlun_link(),
go ahead and use target_fabric_port_item_ops directly instead.

Cc: Christoph Hellwig <h...@lst.de>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_fabric_configfs.c | 13 -
 drivers/target/target_core_tpg.c |  1 -
 include/target/target_core_base.h|  2 --
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index 2cbaecd..e9e917c 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -65,6 +65,8 @@
pr_debug("Setup generic %s\n", __stringify(_name)); \
 }
 
+static struct configfs_item_operations target_fabric_port_item_ops;
+
 /* Start of tfc_tpg_mappedlun_cit */
 
 static int target_fabric_mappedlun_link(
@@ -72,19 +74,20 @@ static int target_fabric_mappedlun_link(
struct config_item *lun_ci)
 {
struct se_dev_entry *deve;
-   struct se_lun *lun = container_of(to_config_group(lun_ci),
-   struct se_lun, lun_group);
+   struct se_lun *lun;
struct se_lun_acl *lacl = container_of(to_config_group(lun_acl_ci),
struct se_lun_acl, se_lun_group);
struct se_portal_group *se_tpg;
struct config_item *nacl_ci, *tpg_ci, *tpg_ci_s, *wwn_ci, *wwn_ci_s;
bool lun_access_ro;
 
-   if (lun->lun_link_magic != SE_LUN_LINK_MAGIC) {
-   pr_err("Bad lun->lun_link_magic, not a valid lun_ci pointer:"
-   " %p to struct lun: %p\n", lun_ci, lun);
+   if (!lun_ci->ci_type ||
+   lun_ci->ci_type->ct_item_ops != _fabric_port_item_ops) {
+   pr_err("Bad lun_ci, not a valid lun_ci pointer: %p\n", lun_ci);
return -EFAULT;
}
+   lun = container_of(to_config_group(lun_ci), struct se_lun, lun_group);
+
/*
 * Ensure that the source port exists
 */
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 74893ac..6b20b9f 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -584,7 +584,6 @@ struct se_lun *core_tpg_alloc_lun(
return ERR_PTR(-ENOMEM);
}
lun->unpacked_lun = unpacked_lun;
-   lun->lun_link_magic = SE_LUN_LINK_MAGIC;
atomic_set(>lun_acl_count, 0);
init_completion(>lun_ref_comp);
init_completion(>lun_shutdown_comp);
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index c3c14d0..47d9f38 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -701,8 +701,6 @@ struct scsi_port_stats {
 
 struct se_lun {
u64 unpacked_lun;
-#define SE_LUN_LINK_MAGIC  0x7771
-   u32 lun_link_magic;
boollun_shutdown;
boollun_access_ro;
u32 lun_index;
-- 
1.9.1

Re: [PATCH] target: Avoid target_shutdown_sessions loop during queue_depth change

[Nicholas A. Bellinger]

On Thu, 2017-06-01 at 08:57 +0200, Christoph Hellwig wrote:
> How about this slightly easier to read version?

Fine by me.

Applied.

[PATCH-v2] target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

The people who are actively using iblock_execute_write_same_direct() are
doing so in the context of ESX VAAI BlockZero, together with
EXTENDED_COPY and COMPARE_AND_WRITE primitives.

In practice though I've not seen any users of IBLOCK WRITE_SAME for
anything other than VAAI BlockZero, so just using blkdev_issue_zeroout()
when available, and falling back to iblock_execute_write_same() if the
WRITE_SAME buffer contains anything other than zeros should be OK.

(Hook up max_write_zeroes_sectors to signal LBPRZ feature bit in
 target_configure_unmap_from_queue - nab)

Signed-off-by: Christoph Hellwig <h...@lst.de>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Martin K. Petersen <martin.peter...@oracle.com>
Cc: Jens Axboe <ax...@fb.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_device.c |  2 +-
 drivers/target/target_core_iblock.c | 44 +++--
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 8add07f38..a5762e6 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -851,7 +851,7 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib 
*attrib,
attrib->unmap_granularity = q->limits.discard_granularity / block_size;
attrib->unmap_granularity_alignment = q->limits.discard_alignment /
block_size;
-   attrib->unmap_zeroes_data = 0;
+   attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
return true;
 }
 EXPORT_SYMBOL(target_configure_unmap_from_queue);
diff --git a/drivers/target/target_core_iblock.c 
b/drivers/target/target_core_iblock.c
index bb069eb..b204413 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -86,6 +86,7 @@ static int iblock_configure_device(struct se_device *dev)
struct block_device *bd = NULL;
struct blk_integrity *bi;
fmode_t mode;
+   unsigned int max_write_zeroes_sectors;
int ret = -ENOMEM;
 
if (!(ib_dev->ibd_flags & IBDF_HAS_UDEV_PATH)) {
@@ -129,7 +130,11 @@ static int iblock_configure_device(struct se_device *dev)
 * Enable write same emulation for IBLOCK and use 0x as
 * the smaller WRITE_SAME(10) only has a two-byte block count.
 */
-   dev->dev_attrib.max_write_same_len = 0x;
+   max_write_zeroes_sectors = bdev_write_zeroes_sectors(bd);
+   if (max_write_zeroes_sectors)
+   dev->dev_attrib.max_write_same_len = max_write_zeroes_sectors;
+   else
+   dev->dev_attrib.max_write_same_len = 0x;
 
if (blk_queue_nonrot(q))
dev->dev_attrib.is_nonrot = 1;
@@ -415,28 +420,31 @@ static void iblock_end_io_flush(struct bio *bio)
 }
 
 static sense_reason_t
-iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd *cmd)
+iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
 {
struct se_device *dev = cmd->se_dev;
struct scatterlist *sg = >t_data_sg[0];
-   struct page *page = NULL;
-   int ret;
+   unsigned char *buf, zero = 0x00, *p = 
+   int rc, ret;
 
-   if (sg->offset) {
-   page = alloc_page(GFP_KERNEL);
-   if (!page)
-   return TCM_OUT_OF_RESOURCES;
-   sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
- dev->dev_attrib.block_size);
-   }
+   buf = kmap(sg_page(sg)) + sg->offset;
+   if (!buf)
+   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+   /*
+* Fall back to block_execute_write_same() slow-path if
+* incoming WRITE_SAME payload does not contain zeros.
+*/
+   rc = memcmp(buf, p, cmd->data_length);
+   kunmap(sg_page(sg));
+
+   if (rc)
+   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-   ret = blkdev_issue_write_same(bdev,
+   ret = blkdev_issue_zeroout(bdev,
target_to_linux_sector(dev, cmd->t_task_lba),
target_to_linux_sector(dev,
sbc_get_write_same_sectors(cmd)),
-   GFP_KERNEL, page ? page : sg_page(sg));
-   if (page)
-   __free_page(page);
+   GFP_KERNEL, false);
if (ret)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
@@ -472,8 +480,10 @@ static void iblock_end_io_flush(struct bio *bio)
return TCM_INVALID_CDB_FIELD;
}
 
-   if (bdev_write_same(bdev)

[PATCH] target: Avoid target_shutdown_sessions loop during queue_depth change

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

When target_shutdown_sessions() is invoked to shutdown all active
sessions associated with a se_node_acl when se_node_acl->queue_depth
is changed via core_tpg_set_initiator_node_queue_depth(), it's
possible that new connections reconnect immediately after explicit
shutdown occurs via target_shutdown_sessions().

Which means it's possible for the newly reconnected session with
the proper queue_depth can be shutdown multiple times when
target_shutdown_sessions() loops to drain all active sessions
for all cases.

This was regression was introduced by:

  commit bc6e6bb470eda42f44bcac96c261cff1216577b3
  Author: Christoph Hellwig <h...@lst.de>
  Date:   Mon May 2 15:45:19 2016 +0200

  target: consolidate and fix session shutdown

To avoid this case, instead move sessions into a local list and
avoid draining the same session multiple times when invoked via
core_tpg_set_initiator_node_queue_depth(), but still loop during
normal se_node_acl delete until all associated sessions have
been shutdown.

Cc: Christoph Hellwig <h...@lst.de>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_tpg.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 310d9e5..7a7c2b5 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -336,10 +336,11 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(
return acl;
 }
 
-static void target_shutdown_sessions(struct se_node_acl *acl)
+static void target_shutdown_sessions(struct se_node_acl *acl, bool do_restart)
 {
struct se_session *sess;
unsigned long flags;
+   LIST_HEAD(tmp_list);
 
 restart:
spin_lock_irqsave(>nacl_sess_lock, flags);
@@ -347,14 +348,22 @@ static void target_shutdown_sessions(struct se_node_acl 
*acl)
if (sess->sess_tearing_down)
continue;
 
+   list_move_tail(>sess_acl_list, _list);
+   }
+   spin_unlock_irqrestore(>nacl_sess_lock, flags);
+
+   if (list_empty(_list))
+   return;
+
+   list_for_each_entry(sess, _list, sess_acl_list) {
list_del_init(>sess_acl_list);
-   spin_unlock_irqrestore(>nacl_sess_lock, flags);
 
if (acl->se_tpg->se_tpg_tfo->close_session)
acl->se_tpg->se_tpg_tfo->close_session(sess);
-   goto restart;
}
-   spin_unlock_irqrestore(>nacl_sess_lock, flags);
+
+   if (do_restart)
+   goto restart;
 }
 
 void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
@@ -367,7 +376,7 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl 
*acl)
list_del(>acl_list);
mutex_unlock(>acl_node_mutex);
 
-   target_shutdown_sessions(acl);
+   target_shutdown_sessions(acl, true);
 
target_put_nacl(acl);
/*
@@ -414,7 +423,7 @@ int core_tpg_set_initiator_node_queue_depth(
/*
 * Shutdown all pending sessions to force session reinstatement.
 */
-   target_shutdown_sessions(acl);
+   target_shutdown_sessions(acl, false);
 
pr_debug("Successfully changed queue depth to: %d for Initiator"
" Node: %s on %s Target Portal Group: %u\n", acl->queue_depth,
-- 
1.9.1

Re: [PATCH 2/8] target: remove iblock WRITE_SAME passthrough support

[Nicholas A. Bellinger]

Hey HCH & Jens,

Is this already queued up for v4.13 to address the missing LBPRZ feature
bit..?

If not, I'll happy to take it via target-pending along with the
following to re-enable it via max_write_zeroes_sectors.

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index d2f089c..e7caf78 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -851,7 +851,7 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib 
*attrib,
attrib->unmap_granularity = q->limits.discard_granularity / block_size;
attrib->unmap_granularity_alignment = q->limits.discard_alignment /
block_size;
-   attrib->unmap_zeroes_data = 0;
+   attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
return true;
 }
 EXPORT_SYMBOL(target_configure_unmap_from_queue);

Any objections..?

On Tue, 2017-04-11 at 22:30 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2017-04-10 at 18:08 +0200, Christoph Hellwig wrote:
> > Use the pscsi driver to support arbitrary command passthrough
> > instead.
> > 
> 
> The people who are actively using iblock_execute_write_same_direct() are
> doing so in the context of ESX VAAI BlockZero, together with
> EXTENDED_COPY and COMPARE_AND_WRITE primitives.  Just using PSCSI is not
> an option for them.
> 
> In practice though I've not seen any users of IBLOCK WRITE_SAME for
> anything other than VAAI BlockZero, so just using blkdev_issue_zeroout()
> when available, and falling back to iblock_execute_write_same() if the
> WRITE_SAME buffer contains anything other than zeros should be OK.
> 
> How about something like the following below..?
> 
> This would bring parity to how blkdev_issue_write_same() works atm wrt
> to synchronous bio completions.  However, most folks with a raw
> make_request or blk-mq backend driver that supports multiple GB/sec of
> zero bandwidth end up changing IBLOCK to support asynchronous
> REQ_WRITE_SAME completions anyways.
> 
> I'd be happy to add support for that using __blkdev_issue_zeroout() once
> the basic conversion is in place.
> 
> From ff74012eaff38f9fa0d74aca60507b9964f484ce Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> Date: Tue, 11 Apr 2017 22:21:47 -0700
> Subject: [PATCH] target/iblock: Convert WRITE_SAME to blkdev_issue_zeroout
> 
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/target/target_core_iblock.c | 44 
> +++--
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/target/target_core_iblock.c 
> b/drivers/target/target_core_iblock.c
> index d316ed5..5bfde20 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -86,6 +86,7 @@ static int iblock_configure_device(struct se_device *dev)
>   struct block_device *bd = NULL;
>   struct blk_integrity *bi;
>   fmode_t mode;
> + unsigned int max_write_zeroes_sectors;
>   int ret = -ENOMEM;
>  
>   if (!(ib_dev->ibd_flags & IBDF_HAS_UDEV_PATH)) {
> @@ -129,7 +130,11 @@ static int iblock_configure_device(struct se_device *dev)
>* Enable write same emulation for IBLOCK and use 0x as
>* the smaller WRITE_SAME(10) only has a two-byte block count.
>*/
> - dev->dev_attrib.max_write_same_len = 0x;
> + max_write_zeroes_sectors = bdev_write_zeroes_sectors(bd);
> + if (max_write_zeroes_sectors)
> + dev->dev_attrib.max_write_same_len = max_write_zeroes_sectors;
> + else
> + dev->dev_attrib.max_write_same_len = 0x;
>  
>   if (blk_queue_nonrot(q))
>   dev->dev_attrib.is_nonrot = 1;
> @@ -415,28 +420,31 @@ static void iblock_end_io_flush(struct bio *bio)
>  }
>  
>  static sense_reason_t
> -iblock_execute_write_same_direct(struct block_device *bdev, struct se_cmd 
> *cmd)
> +iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct scatterlist *sg = >t_data_sg[0];
> - struct page *page = NULL;
> - int ret;
> + unsigned char *buf, zero = 0x00, *p = 
> + int rc, ret;
>  
> - if (sg->offset) {
> - page = alloc_page(GFP_KERNEL);
> - if (!page)
> - return TCM_OUT_OF_RESOURCES;
> - sg_copy_to_buffer(sg, cmd->t_data_nents, page_address(page),
> -   dev->dev_attrib.block_size);
> - }
> + buf = kmap(sg_page(sg)) + sg->offset;
> + if (!buf)
> + return 

Re: [PATCH] iscsi: Fix a sleep-in-atomic bug

[Nicholas A. Bellinger]

Hi Jia-Ju,

On Wed, 2017-05-31 at 11:26 +0800, Jia-Ju Bai wrote:
> The driver may sleep under a spin lock, and the function call path is:
> iscsit_tpg_enable_portal_group (acquire the lock by spin_lock)
>   iscsi_update_param_value
> kstrdup(GFP_KERNEL) --> may sleep
> 
> To fix it, the "GFP_KERNEL" is replaced with "GFP_ATOMIC".
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/target/iscsi/iscsi_target_parameters.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Btw, the use of tpg->tpg_state_lock in iscsit_tpg_enable_portal_group()
while checking existing state and calling iscsi_update_param_value() is
not necessary, since lio_target_tpg_enable_store() is already holding
iscsit_get_tpg() -> tpg->tpg_access_lock.

How about the following instead to only take tpg->tpg_state_lock when
updating tpg->tpg_state instead..?

diff --git a/drivers/target/iscsi/iscsi_target_tpg.c 
b/drivers/target/iscsi/iscsi_target_tpg.c
index 2e7e08d..abaabba 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -311,11 +311,9 @@ int iscsit_tpg_enable_portal_group(struct 
iscsi_portal_group *tpg)
struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
int ret;
 
-   spin_lock(>tpg_state_lock);
if (tpg->tpg_state == TPG_STATE_ACTIVE) {
pr_err("iSCSI target portal group: %hu is already"
" active, ignoring request.\n", tpg->tpgt);
-   spin_unlock(>tpg_state_lock);
return -EINVAL;
}
/*
@@ -324,10 +322,8 @@ int iscsit_tpg_enable_portal_group(struct 
iscsi_portal_group *tpg)
 * is enforced (as per default), and remove the NONE option.
 */
param = iscsi_find_param_from_key(AUTHMETHOD, tpg->param_list);
-   if (!param) {
-   spin_unlock(>tpg_state_lock);
+   if (!param)
return -EINVAL;
-   }
 
if (tpg->tpg_attrib.authentication) {
if (!strcmp(param->value, NONE)) {
@@ -341,6 +337,7 @@ int iscsit_tpg_enable_portal_group(struct 
iscsi_portal_group *tpg)
goto err;
}
 
+   spin_lock(>tpg_state_lock);
tpg->tpg_state = TPG_STATE_ACTIVE;
spin_unlock(>tpg_state_lock);
 
@@ -353,7 +350,6 @@ int iscsit_tpg_enable_portal_group(struct 
iscsi_portal_group *tpg)
return 0;
 
 err:
-   spin_unlock(>tpg_state_lock);
return ret;
 }

Re: [PATCH] tcmu: Add fifo type waiter list support to avoid starvation

[Nicholas A. Bellinger]

Hey MNC,

Any comments on this..?

It's been sitting on the list for a while now..  ;)

On Fri, 2017-05-05 at 10:51 +0800, lixi...@cmss.chinamobile.com wrote:
> From: Xiubo Li 
> 
> The fifo type waiter list will hold the udevs who are waiting for the
> blocks from the data global pool. The unmap thread will try to feed the
> first udevs in waiter list, if the global free blocks available are
> not enough, it will stop traversing the list and abort waking up the
> others.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 82 
> +++
>  1 file changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 9045837..10c99e7 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -97,6 +97,7 @@ struct tcmu_hba {
>  
>  struct tcmu_dev {
>   struct list_head node;
> + struct list_head waiter;
>  
>   struct se_device se_dev;
>  
> @@ -123,7 +124,7 @@ struct tcmu_dev {
>   wait_queue_head_t wait_cmdr;
>   struct mutex cmdr_lock;
>  
> - bool waiting_global;
> + uint32_t waiting_blocks;
>   uint32_t dbi_max;
>   uint32_t dbi_thresh;
>   DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS);
> @@ -165,6 +166,10 @@ struct tcmu_cmd {
>  static DEFINE_MUTEX(root_udev_mutex);
>  static LIST_HEAD(root_udev);
>  
> +/* The data blocks global pool waiter list */
> +static DEFINE_MUTEX(root_udev_waiter_mutex);
> +static LIST_HEAD(root_udev_waiter);
> +
>  static atomic_t global_db_count = ATOMIC_INIT(0);
>  
>  static struct kmem_cache *tcmu_cmd_cache;
> @@ -195,6 +200,11 @@ enum tcmu_multicast_groups {
>  #define tcmu_cmd_set_dbi(cmd, index) ((cmd)->dbi[(cmd)->dbi_cur++] = (index))
>  #define tcmu_cmd_get_dbi(cmd) ((cmd)->dbi[(cmd)->dbi_cur++])
>  
> +static inline bool is_in_waiter_list(struct tcmu_dev *udev)
> +{
> + return !!udev->waiting_blocks;
> +}
> +
>  static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd, uint32_t len)
>  {
>   struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
> @@ -250,8 +260,6 @@ static bool tcmu_get_empty_blocks(struct tcmu_dev *udev,
>  {
>   int i;
>  
> - udev->waiting_global = false;
> -
>   for (i = tcmu_cmd->dbi_cur; i < tcmu_cmd->dbi_cnt; i++) {
>   if (!tcmu_get_empty_block(udev, tcmu_cmd))
>   goto err;
> @@ -259,9 +267,7 @@ static bool tcmu_get_empty_blocks(struct tcmu_dev *udev,
>   return true;
>  
>  err:
> - udev->waiting_global = true;
> - /* Try to wake up the unmap thread */
> - wake_up(_wait);
> + udev->waiting_blocks += tcmu_cmd->dbi_cnt - i;
>   return false;
>  }
>  
> @@ -671,6 +677,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct 
> tcmu_cmd *tcmu_cmd,
>  
>   while (!is_ring_space_avail(udev, tcmu_cmd, command_size, data_length)) 
> {
>   int ret;
> + bool is_waiting_blocks = !!udev->waiting_blocks;
>   DEFINE_WAIT(__wait);
>  
>   prepare_to_wait(>wait_cmdr, &__wait, TASK_INTERRUPTIBLE);
> @@ -688,6 +695,18 @@ static inline size_t tcmu_cmd_get_cmd_size(struct 
> tcmu_cmd *tcmu_cmd,
>   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   }
>  
> + /*
> +  * Try to insert the current udev to waiter list and
> +  * then wake up the unmap thread
> +  */
> + if (is_waiting_blocks) {
> + mutex_lock(_udev_waiter_mutex);
> + list_add_tail(>waiter, _udev_waiter);
> + mutex_unlock(_udev_waiter_mutex);
> +
> + wake_up(_wait);
> + }
> +
>   mutex_lock(>cmdr_lock);
>  
>   /* We dropped cmdr_lock, cmd_head is stale */
> @@ -902,8 +921,6 @@ static unsigned int tcmu_handle_completions(struct 
> tcmu_dev *udev)
>   if (mb->cmd_tail == mb->cmd_head)
>   del_timer(>timeout); /* no more pending cmds */
>  
> - wake_up(>wait_cmdr);
> -
>   return handled;
>  }
>  
> @@ -996,7 +1013,17 @@ static int tcmu_irqcontrol(struct uio_info *info, s32 
> irq_on)
>   struct tcmu_dev *tcmu_dev = container_of(info, struct tcmu_dev, 
> uio_info);
>  
>   mutex_lock(_dev->cmdr_lock);
> - tcmu_handle_completions(tcmu_dev);
> + /*
> +  * If the current udev is also in waiter list, this will
> +  * make sure that the other waiters in list be feeded ahead
> +  * of it.
> +  */
> + if (is_in_waiter_list(tcmu_dev)) {
> + wake_up(_wait);
> + } else {
> + tcmu_handle_completions(tcmu_dev);
> + wake_up(_dev->wait_cmdr);
> + }
>   mutex_unlock(_dev->cmdr_lock);
>  
>   return 0;
> @@ -1231,7 +1258,7 @@ static int tcmu_configure_device(struct se_device *dev)
>   udev->data_off = CMDR_SIZE;
>   

[GIT PULL] target fixes for v4.12-rc4

[Nicholas A. Bellinger]

Hello Linus,

Here are the target-pending fixes for v4.12-rc4.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git master

This includes:

  - ibmviscsis ABORT_TASK handling fixes that missed the v4.12 merge 
window. (Bryant Ly and Michael Cyr)
  - Re-add a target-core check enforcing WRITE overflow reject that was 
relaxed in v4.3, to avoid unsupported iscsi-target immediate data 
overflow. (nab)
  - Fix a target-core-user OOPs during device removal. (MNC + Bryant Ly)
  - Fix a long standing iscsi-target potential issue where kthread exit 
did not wait for kthread_should_stop(). (Jiang Yi)
  - Fix a iscsi-target v3.12.y regression OOPs involving initial login 
PDU processing during asynchronous TCP connection close. (MNC + nab)

This is a little larger than usual for an -rc4, primarily due to the
iscsi-target v3.12.y regression OOPs bug-fix.

However, it's an important patch as MNC + Hannes where both able to
trigger it using a reduced iscsi initiator login timeout combined with a
backend taking a long time to complete I/Os during iscsi login driven
session reinstatement.

Note the trailing two patches have a recent CommitDate, as the second to
last was updated today to include MNC's tested-by and reviewed-by tags.
The patches themselves has not changed though, and both have been run
through manual and automated regression testing from multiple parties
over the last 7 days.

Thank you,

--nab

Bryant G. Ly (2):
  ibmvscsis: Clear left-over abort_cmd pointers
  ibmvscsis: Fix the incorrect req_lim_delta

Jiang Yi (1):
  iscsi-target: Always wait for kthread_should_stop() before kthread
exit

Mike Christie (1):
  tcmu: fix crash during device removal

Nicholas Bellinger (2):
  target: Re-add check to reject control WRITEs with overflow data
  iscsi-target: Fix initial login PDU asynchronous socket close OOPs

 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c  |  27 -
 drivers/target/iscsi/iscsi_target.c   |  30 -
 drivers/target/iscsi/iscsi_target_erl0.c  |   6 +-
 drivers/target/iscsi/iscsi_target_erl0.h  |   2 +-
 drivers/target/iscsi/iscsi_target_login.c |   4 +
 drivers/target/iscsi/iscsi_target_nego.c  | 194 --
 drivers/target/target_core_transport.c|  23 +++-
 drivers/target/target_core_user.c |  46 +--
 include/target/iscsi/iscsi_target_core.h  |   1 +
 9 files changed, 241 insertions(+), 92 deletions(-)

Re: [PATCH] iscsi-target: Fix initial login PDU asynchronous socket close OOPs

[Nicholas A. Bellinger]

On Wed, 2017-05-31 at 15:28 -0500, Mike Christie wrote:
> On 05/30/2017 11:58 PM, Nicholas A. Bellinger wrote:
> > Hey MNC,
> > 
> > On Fri, 2017-05-26 at 22:14 -0500, Mike Christie wrote:
> >> Thanks for the patch.



> >> The patch fixes the crash for me. However, is there a possible
> >> regression where if the initiator attempts new relogins we could run out
> >> of memory? With the old code, we would free the login attempts resources
> >> at this time, but with the new code the initiator will send more login
> >> attempts and so we just keep allocating more memory for each attempt
> >> until we run out or the login is finally able to complete.
> > 
> > AFAICT, no. For the two cases in question:
> > 
> >  - Initial login request PDU processing done within iscsi_np kthread
> > context in iscsi_target_start_negotiation(), and
> >  - subsequent login request PDU processing done by delayed work-queue
> > kthread context in iscsi_target_do_login_rx() 
> > 
> > this patch doesn't change how aggressively connection cleanup happens
> > for failed login attempts in the face of new connection login attempts
> > for either case.
> > 
> > For the first case when iscsi_np process context invokes
> > iscsi_target_start_negotiation() -> iscsi_target_do_login() ->
> > iscsi_check_for_session_reinstatement() to wait for backend I/O to
> > complete, it still blocks other new connections from being accepted on
> > the specific iscsi_np process context.
> > 
> > This patch doesn't change this behavior.
> > 
> > What it does change is when the host closes the connection and
> > iscsi_target_sk_state_change() gets invoked, iscsi_np process context
> > waits for iscsi_check_for_session_reinstatement() to complete before
> > releasing the connection resources.
> > 
> > However since iscsi_np process context is blocked, new connections won't
> > be accepted until the new connection forcing session reinstatement
> > finishes waiting for outstanding backend I/O to complete.
> 
> I was seeing this. My original mail asked about iscsi login resources
> incorrectly, but like you said we do not get that far. I get a giant
> backlog (1 connection request per 5 seconds that we waited) of tcp level
> connection requests and drops. When the wait is done I get a flood of
> "iSCSI Login negotiation failed" due to the target handling all those
> now stale requests/drops.

Ah, I see what you mean.  The TCP backlog = 256 default can fill up when
a small host side login timeout is used while iscsi_np is blocked
waiting for session reinstatement to complete.

> 
> If we do not care about the memory use at the network level for this
> case (it seems like a little and reconnects are not aggressive), then
> patch works ok for me. I am guessing it gets nasty to handle, so maybe
> not worth it to handle right now? 

Yeah, since it's a issue separate from root cause here, getting this
merged first makes sense.

> I tried to do it in my patch which is why it got all crazy with the 
> waits/wakeups :)
> 

One option to consider is to immediately queue into delayed work-queue
context from iscsi_target_start_negotiation() instead of doing the
iscsi_target_do_login() and session reinstatement from iscsi_np context.

Just taking a quick look, this seems like it would be a pretty
straight-forward change..

> Thanks, and you can add a tested-by or reviewed-by from me.

Great, thanks MNC.

Will send out a PULL request for -rc4 shortly.

Re: [PATCH] iscsi-target: Fix initial login PDU asynchronous socket close OOPs

[Nicholas A. Bellinger]

Hey MNC,

On Fri, 2017-05-26 at 22:14 -0500, Mike Christie wrote:
> Thanks for the patch.
> 

Btw, after running DATERA's internal longevity and scale tests across
~20 racks on v4.1.y with this patch over the long weekend, there haven't
been any additional regressions.

> On 05/26/2017 12:32 AM, Nicholas A. Bellinger wrote:
> >  
> > -   state = iscsi_target_sk_state_check(sk);
> > -   write_unlock_bh(>sk_callback_lock);
> > -
> > -   pr_debug("iscsi_target_sk_state_change: state: %d\n", state);
> > +   orig_state_change(sk);
> >  
> > -   if (!state) {
> > -   pr_debug("iscsi_target_sk_state_change got failed state\n");
> > -   schedule_delayed_work(>login_cleanup_work, 0);
> 
> I think login_cleanup_work is no longer used so you can also remove it
> and its code.

Yep, since this needs to goto stable, I left that part out for now..

Will take care of that post -rc4.

> 
> The patch fixes the crash for me. However, is there a possible
> regression where if the initiator attempts new relogins we could run out
> of memory? With the old code, we would free the login attempts resources
> at this time, but with the new code the initiator will send more login
> attempts and so we just keep allocating more memory for each attempt
> until we run out or the login is finally able to complete.

AFAICT, no. For the two cases in question:

 - Initial login request PDU processing done within iscsi_np kthread
context in iscsi_target_start_negotiation(), and
 - subsequent login request PDU processing done by delayed work-queue
kthread context in iscsi_target_do_login_rx() 

this patch doesn't change how aggressively connection cleanup happens
for failed login attempts in the face of new connection login attempts
for either case.

For the first case when iscsi_np process context invokes
iscsi_target_start_negotiation() -> iscsi_target_do_login() ->
iscsi_check_for_session_reinstatement() to wait for backend I/O to
complete, it still blocks other new connections from being accepted on
the specific iscsi_np process context.

This patch doesn't change this behavior.

What it does change is when the host closes the connection and
iscsi_target_sk_state_change() gets invoked, iscsi_np process context
waits for iscsi_check_for_session_reinstatement() to complete before
releasing the connection resources.

However since iscsi_np process context is blocked, new connections won't
be accepted until the new connection forcing session reinstatement
finishes waiting for outstanding backend I/O to complete.

For the second case of subsequent non initial login request PDUs handled
within delayed work-queue process context, AFAICT this patch doesn't
change the original behavior either..

Namely when iscsi_target_do_login_rx() is active and host closes the
connection causing iscsi_target_sk_state_change() to be invoked, it
still checks for LOGIN_FLAGS_READ_ACTIVE and doesn't queue shutdown to
occur.

As per the original logic preceding this change, it continues to wait
for iscsi_target_do_login_rx() to complete in delayed work-queue
context, unless sock_recvmsg() returns a failure (which it should once
TCP_CLOSE occurs) or times out via iscsi_target_login_timeout().  Once
the failure is detected from iscsi_target_do_login_rx(), the remaining
connection resources are related from there.

That said, was there another case you had in mind..?

[PATCH] iscsi-target: Fix initial login PDU asynchronous socket close OOPs

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch fixes a OOPs originally introduced by:

   commit bb048357dad6d604520c91586334c9c230366a14
   Author: Nicholas Bellinger <n...@linux-iscsi.org>
   Date:   Thu Sep 5 14:54:04 2013 -0700

   iscsi-target: Add sk->sk_state_change to cleanup after TCP failure

which would trigger a NULL pointer dereference when a TCP connection
was closed asynchronously via iscsi_target_sk_state_change(), but only
when the initial PDU processing in iscsi_target_do_login() from iscsi_np
process context was blocked waiting for backend I/O to complete.

To address this issue, this patch makes the following changes.

First, it introduces some common helper functions used for checking
socket closing state, checking login_flags, and atomically checking
socket closing state + setting login_flags.

Second, it introduces a LOGIN_FLAGS_INITIAL_PDU bit to know when a TCP
connection has dropped via iscsi_target_sk_state_change(), but the
initial PDU processing within iscsi_target_do_login() in iscsi_np
context is still running.  For this case, it sets LOGIN_FLAGS_CLOSED,
but doesn't invoke schedule_delayed_work().

The original NULL pointer dereference case reported by MNC is now handled
by iscsi_target_do_login() doing a iscsi_target_sk_check_close() before
transitioning to FFP to determine when the socket has already closed,
or iscsi_target_start_negotiation() if the login needs to exchange
more PDUs (eg: iscsi_target_do_login returned 0) but the socket has
closed.  For both of these cases, the cleanup up of remaining connection
resources will occur in iscsi_target_start_negotiation() from iscsi_np
process context once the failure is detected.

Finally, to handle to case where iscsi_target_sk_state_change() is
called after the initial PDU procesing is complete, it now invokes
conn->login_work -> iscsi_target_do_login_rx() to perform cleanup once
existing iscsi_target_sk_check_close() checks detect connection failure.
For this case, the cleanup of remaining connection resources will occur
in iscsi_target_do_login_rx() from delayed workqueue process context
once the failure is detected.

Reported-by: Mike Christie <mchri...@redhat.com>
Cc: Mike Christie <mchri...@redhat.com>
Reported-by: Hannes Reinecke <h...@suse.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Sagi Grimberg <s...@grimberg.me>
Cc: Varun Prakash <va...@chelsio.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_nego.c | 194 +--
 include/target/iscsi/iscsi_target_core.h |   1 +
 2 files changed, 133 insertions(+), 62 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c 
b/drivers/target/iscsi/iscsi_target_nego.c
index 7ccc9c1..6f88b31 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -493,14 +493,60 @@ static void iscsi_target_restore_sock_callbacks(struct 
iscsi_conn *conn)
 
 static int iscsi_target_do_login(struct iscsi_conn *, struct iscsi_login *);
 
-static bool iscsi_target_sk_state_check(struct sock *sk)
+static bool __iscsi_target_sk_check_close(struct sock *sk)
 {
if (sk->sk_state == TCP_CLOSE_WAIT || sk->sk_state == TCP_CLOSE) {
-   pr_debug("iscsi_target_sk_state_check: 
TCP_CLOSE_WAIT|TCP_CLOSE,"
+   pr_debug("__iscsi_target_sk_check_close: 
TCP_CLOSE_WAIT|TCP_CLOSE,"
"returning FALSE\n");
-   return false;
+   return true;
}
-   return true;
+   return false;
+}
+
+static bool iscsi_target_sk_check_close(struct iscsi_conn *conn)
+{
+   bool state = false;
+
+   if (conn->sock) {
+   struct sock *sk = conn->sock->sk;
+
+   read_lock_bh(>sk_callback_lock);
+   state = (__iscsi_target_sk_check_close(sk) ||
+test_bit(LOGIN_FLAGS_CLOSED, >login_flags));
+   read_unlock_bh(>sk_callback_lock);
+   }
+   return state;
+}
+
+static bool iscsi_target_sk_check_flag(struct iscsi_conn *conn, unsigned int 
flag)
+{
+   bool state = false;
+
+   if (conn->sock) {
+   struct sock *sk = conn->sock->sk;
+
+   read_lock_bh(>sk_callback_lock);
+   state = test_bit(flag, >login_flags);
+   read_unlock_bh(>sk_callback_lock);
+   }
+   return state;
+}
+
+static bool iscsi_target_sk_check_and_clear(struct iscsi_conn *conn, unsigned 
int flag)
+{
+   bool state = false;
+
+   if (conn->sock) {
+   struct sock *sk = conn->sock->sk;
+
+   write_lock_bh(>sk_callback_lock);
+   state = (__iscsi_target_sk_check_close(sk) ||
+test_bit(LOGIN_FLAGS_CLOSED, >login_flags));
+   if (!state)
+   clear_

Re: [PATCH] ibmvscsis: Enable Logical Partition Migration Support

[Nicholas A. Bellinger]

On Tue, 2017-05-16 at 17:49 -0500, Bryant G. Ly wrote:
> From: Michael Cyr 
> 
> Changes to support a new mechanism from phyp to better synchronize the
> logical partition migration (LPM) of the client partition.
> This includes a new VIOCTL to register that we support this new
> functionality, and 2 new Transport Event types, and finally another
> new VIOCTL to let phyp know once we're ready for the Suspend.
> 
> Signed-off-by: Michael Cyr 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 148 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |  25 +-
>  drivers/scsi/ibmvscsi_tgt/libsrp.h   |   5 +-
>  3 files changed, 162 insertions(+), 16 deletions(-)
> 

Applied to target-pending/for-next.

Thanks Bryant & Co.

Re: [PATCH] target: remove dead code

[Nicholas A. Bellinger]

On Thu, 2017-05-11 at 13:39 -0700, Tyrel Datwyler wrote:
> On 05/09/2017 02:46 PM, Gustavo A. R. Silva wrote:
> > Local variable _ret_ is assigned to a constant value and it is never
> > updated again. Remove this variable and the dead code it guards.
> > 
> > Addresses-Coverity-ID: 140761
> > Signed-off-by: Gustavo A. R. Silva 
> > ---
> 
> Reviewed-by: Tyrel Datwyler 
> 

Applied to target-pending/for-next.

Thanks Gustavo + Tyrel.

Re: [PATCH 13/25] tcm_qla2xxx: Do not allow aborted cmd to advance.

[Nicholas A. Bellinger]

On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> From: Quinn Tran <quinn.t...@cavium.com>
> 
> In case of hardware queue full, commands can loop between
> TCM stack and tcm_qla2xx shim layers for retry. While command
> is waiting for retry, task mgmt can get ahead and abort the
> cmmand that encountered queue full condition. Fix this by
> dropping the command, if task mgmt has already started the
> command free process.
> 
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Quinn Tran <quinn.t...@cavium.com>
> Signed-off-by: Himanshu Madhani <himanshu.madh...@cavium.com>
> ---
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 7443e4efa3ae..07f8ad001bcb 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -686,6 +686,20 @@ static int tcm_qla2xxx_queue_status(struct se_cmd 
> *se_cmd)
>   struct qla_tgt_cmd, se_cmd);
>   int xmit_type = QLA_TGT_XMIT_STATUS;
>  
> + if (cmd->aborted) {
> + /*
> +  * Cmd can loop during Q-full. tcm_qla2xxx_aborted_task
> +  * can get ahead of this cmd. tcm_qla2xxx_aborted_task
> +  * already kick start the free.
> +  */
> + pr_debug(
> + "queue_data_in aborted cmd[%p] refcount %d transport_state 
> %x, t_state %x, se_cmd_flags %x\n",
> + cmd, kref_read(>se_cmd.cmd_kref),
> + cmd->se_cmd.transport_state, cmd->se_cmd.t_state,
> + cmd->se_cmd.se_cmd_flags);
> + return 0;
> + }
> +
>   cmd->bufflen = se_cmd->data_length;
>   cmd->sg = NULL;
>   cmd->sg_cnt = 0;

As your comment above mentions, there is a known issue in target-core
when queue-full occurs repeatably and a se_cmd descriptor is explicitly
stopped via session shutdown, TMR ABORT_TASK / LUN_RESET or otherwise.

We hit this scenario a while back in iser-target with iw_cxgb hw, which
due to a separate bug was constantly triggering queue-full under load to
uncover this same case.

So I'm still considering the different approaches to address this in
target-core proper, but don't have a problem with merging this as-is as
it won't logically conflict with any of those changes.

That said:

Acked-by: Nicholas Bellinger <n...@linux-iscsi.org>

Re: [PATCH 15/25] qla2xxx: Convert 32-bit LUN usage to 64-bit

[Nicholas A. Bellinger]

On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> From: Quinn Tran <quinn.t...@cavium.com>
> 
> Convert 32bit LUN field to 64bit LUN.
> 
> Signed-off-by: Quinn Tran <quinn.t...@cavium.com>
> Signed-off-by: Himanshu Madhani <himanshu.madh...@cavium.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c  | 30 +-
>  drivers/scsi/qla2xxx/qla_target.h  |  4 ++--
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |  2 +-
>  3 files changed, 16 insertions(+), 20 deletions(-)
> 

Aside from the typo to hard-code LUN=0 in __qlt_24xx_handle_abts() Bart
mentioned earlier, looks fine.

Acked-by: Nicholas Bellinger <n...@linux-iscsi.org>

Re: [PATCH 20/25] qla2xxx: Remove redundant code

[Nicholas A. Bellinger]

On Fri, 2017-05-19 at 23:43 +, Bart Van Assche wrote:
> On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> > From: Quinn Tran <quinn.t...@cavium.com>
> > 
> > During ABTS or Abort task, qla2xxx does a pre-search for
> > the se_cmd, based on command's tag. The same search is
> > performed by TCM. Remove the extra search from qla2xxx.
> > 
> > Signed-off-by: Quinn Tran <quinn.t...@cavium.com>
> > Signed-off-by: Himanshu Madhani <himanshu.madh...@cavium.com>
> > ---
> >  drivers/scsi/qla2xxx/qla_target.c | 29 -
> >  1 file changed, 4 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c 
> > b/drivers/scsi/qla2xxx/qla_target.c
> > index 21e8993baf4b..b8e609ae6cff 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -1836,34 +1836,13 @@ static int __qlt_24xx_handle_abts(struct 
> > scsi_qla_host *vha,
> > struct abts_recv_from_24xx *abts, struct fc_port *sess)
> >  {
> > struct qla_hw_data *ha = vha->hw;
> > -   struct se_session *se_sess = sess->se_sess;
> > struct qla_tgt_mgmt_cmd *mcmd;
> > -   struct se_cmd *se_cmd;
> > int rc;
> > -   bool found_lun = false;
> > -   unsigned long flags;
> > -
> > -   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
> > -   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
> > -   if (se_cmd->tag == abts->exchange_addr_to_abort) {
> > -   found_lun = true;
> > -   break;
> > -   }
> > -   }
> > -   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
> >  
> > -   /* cmd not in LIO lists, look in qla list */
> > -   if (!found_lun) {
> > -   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
> > -   /* send TASK_ABORT response immediately */
> > -   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
> > -   return 0;
> > -   } else {
> > -   ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
> > -   "unable to find cmd in driver or LIO for tag 
> > 0x%x\n",
> > -   abts->exchange_addr_to_abort);
> > -   return -ENOENT;
> > -   }
> > +   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
> > +   /* send TASK_ABORT response immediately */
> > +   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
> > +   return 0;
> > }
> >  
> > ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
> 
> Hello Himanshu and Quinn,
> 
> Please drop this patch. If a command has already been submitted to the LIO
> core and an ABTS is received then the LIO core should be requested to perform
> the abort. This patch changes the behavior of the qla2xxx target driver such
> that the LIO core is not informed at all if abort_cmd_for_tag() finds the
> command that has to be aborted in one of the command lists maintained by the
> qla2xxx driver. That can lead to the presence of overlapping writes in the
> command set on the target system and hence to data corruption.

This analysis is wrong.

The three lists abort_cmd_for_tag() walks from __qlt_24xx_handle_abts()
are used to track descriptors only before __qlt_do_work() is reached,
and before a descriptor is submitted into tcm_qla2xxx code.

Or rather, the three lists in abort_cmd_for_tag() only contain
qla_tgt_cmd or qla_tgt_sess_op descriptors that have not yet reached
qla_tgt_func_tmpl->handle_cmd() code.

Both qlt_do_work() and qlt_create_sess_from_atio() drop their respective
descriptors from ->cmd_list before dispatching into tcm_qla2xxx ->
target-core, which means there is no way for a descriptor to be part of
internal lists once __qlt_do_work() is called.

That said, the patch is correct and removes the redundant lookup.

Acked-by: Nicholas Bellinger <n...@linux-iscsi.org>

[GIT PULL] target updates for v4.12-rc1

[Nicholas A. Bellinger]

Hi Linus,

Here are the target-pending updates for v4.12-rc1.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next

Things where alot more calm than previously expected.  It's primarily
fixes in various areas, with most of the new functionality centering
around TCMU backend driver work that Xiubo Li has been driving.

Here's the summary on the feature side:

  - Make T10-PI verify configurable for emulated (FILEIO + RD) backends 
(Dmitry Monakhov)
  - Allow target-core/TCMU pass-through to use in-kernel SPC-PR logic
(Bryant Ly + MNC)
  - Add TCMU support for growing ring buffer size (Xiubo Li + MNC)
  - Add TCMU support for global block data pool (Xiubo Li + MNC)

and on the bug-fix side:

  - Fix COMPARE_AND_WRITE non GOOD status handling for READ phase 
failures (Gary Guo + nab)
  - Fix iscsi-target hang with explicitly changing per NodeACL
CmdSN number depth with concurrent login driven session 
reinstatement.  (Gary Guo + nab)
  - Fix ibmvscsis fabric driver ABORT task handling (Bryant Ly)
  - Fix target-core/FILEIO zero length handling (Bart Van Assche)

Also, there was an OOPs introduced with the WRITE_VERIFY changes that I
ended up reverting at the last minute, because as not unusual Bart and I
could not agree on the fix in time for -rc1.  Since it's specific to a
conformance test, it's been reverted for now.

There is a separate patch in the queue to address the underlying control
CDB write overflow regression in >= v4.3 separate from the WRITE_VERIFY
revert here, that will be pushed post -rc1.

Thank you,

--nab

Bart Van Assche (4):
  target: Fix VERIFY and WRITE VERIFY command parsing
  target/fileio: Fix zero-length READ and WRITE handling
  IB/srpt: Fix abort handling
  IB/srpt: Avoid that aborting a command triggers a kernel warning

Bryant G. Ly (3):
  target: Add WRITE_VERIFY_16
  target/user: PGR Support
  ibmvscsis: Do not send aborted task response

Christophe Vu-Brugier (1):
  Documentation/target: add an example script to configure an iSCSI
target

Dmitry Monakhov (2):
  tcm_fileio: Prevent information leak for short reads
  tcm: make pi data verification configurable

Elena Reshetova (1):
  target/iblock: convert iblock_req.pending from atomic_t to refcount_t

Hannes Reinecke (2):
  target: fixup error message in
target_tg_pt_gp_alua_access_type_store()
  target: fixup error message in target_tg_pt_gp_tg_pt_gp_id_store()

Markus Elfring (8):
  iscsi-target: Use kcalloc() in iscsit_allocate_iovecs()
  iscsi-target: Delete error messages for failed memory allocations
  iscsi-target: Improve size determinations in four functions
  target: Use kcalloc() in two functions
  target: Delete error messages for failed memory allocations
  target: Improve size determinations in two functions
  target: Use kmalloc_array() in compare_and_write_callback()
  target: Use kmalloc_array() in transport_kmap_data_sg()

Mike Christie (1):
  tcmu: fix module removal due to stuck thread

Nicholas Bellinger (4):
  target: Fix compare_and_write_callback handling for non GOOD status
  iscsi-target: Set session_fall_back_to_erl0 when forcing reinstatement
  target: Don't force session reset if queue_depth does not change
  Revert "target: Fix VERIFY and WRITE VERIFY command parsing"

Xiubo Li (3):
  tcmu: Add dynamic growing data area feature support
  tcmu: Add global data block pool support
  tcmu: Recalculate the tcmu_cmd size to save cmd area memories

Zhu Lingshan (1):
  target/pr: update PR out action code table

 Documentation/target/target-export-device|  80 
 drivers/infiniband/ulp/srpt/ib_srpt.c|   9 +-
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 114 +++--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |   2 +
 drivers/target/iscsi/iscsi_target.c  |  51 +--
 drivers/target/iscsi/iscsi_target_configfs.c |   1 +
 drivers/target/iscsi/iscsi_target_login.c|   1 +
 drivers/target/target_core_configfs.c|  54 ++-
 drivers/target/target_core_device.c  |  38 ++
 drivers/target/target_core_file.c|  32 +-
 drivers/target/target_core_iblock.c  |  12 +-
 drivers/target/target_core_iblock.h  |   3 +-
 drivers/target/target_core_pr.c  |   2 +-
 drivers/target/target_core_pr.h  |   9 +-
 drivers/target/target_core_pscsi.c   |   3 +-
 drivers/target/target_core_rd.c  |  50 +--
 drivers/target/target_core_sbc.c |  10 +-
 drivers/target/target_core_tpg.c |   7 +
 drivers/target/target_core_transport.c   |   2 +-
 drivers/target/target_core_user.c| 645 +--
 include/scsi/scsi_proto.h|   1 +
 include/target/target_core_backend.h |   1 +
 include/target/target_core_base.h|   1 +
 23 files changed, 859 insertions(+), 269 deletions(-)
 create mode 100755 Documentation/target/target-export-device

[PATCH] target: Re-add check to reject control WRITEs with overflow data

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

During v4.3 when the overflow/underflow check was relaxed by
commit c72c525022:

  commit c72c5250224d475614a00c1d7e54a67f77cd3410
  Author: Roland Dreier <rol...@purestorage.com>
  Date:   Wed Jul 22 15:08:18 2015 -0700

   target: allow underflow/overflow for PR OUT etc. commands

to allow underflow/overflow for Windows compliance + FCP, a
consequence was to allow control CDBs to process overflow
data for iscsi-target with immediate data as well.

As per Roland's original change, continue to allow underflow
cases for control CDBs to make Windows compliance + FCP happy,
but until then explicitly reject all control WRITEs with
overflow following pre v4.3.y logic.

Reported-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: Roland Dreier <rol...@purestorage.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 37f5735..6025935 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1160,15 +1160,28 @@ int transport_dump_vpd_ident(
if (cmd->unknown_data_length) {
cmd->data_length = size;
} else if (size != cmd->data_length) {
-   pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
+   pr_warn_ratelimited("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;
+   if (cmd->data_direction == DMA_TO_DEVICE) {
+   if (cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
+   pr_err_ratelimited("Rejecting 
underflow/overflow"
+  " for WRITE data CDB\n");
+   return TCM_INVALID_CDB_FIELD;
+   }
+   /*
+* Some fabric drivers like iscsi-target still expect to
+* always reject overflow writes.  Reject this case 
until
+* full fabric driver level support for overflow writes
+* is introduced tree-wide.
+*/
+   if (size > cmd->data_length) {
+   pr_err_ratelimited("Rejecting overflow for"
+  " WRITE control CDB\n");
+   return TCM_INVALID_CDB_FIELD;
+   }
}
/*
 * Reject READ_* or WRITE_* with overflow/underflow for
-- 
1.9.1

Re: [PATCH v1] ibmvscsis: Fix cleaning up pointers

[Nicholas A. Bellinger]

On Thu, 2017-05-11 at 10:40 -0500, Bryant G. Ly wrote:
> On 5/9/17 10:45 PM, Nicholas A. Bellinger wrote:
> 
> > On Tue, 2017-05-09 at 11:50 -0500, Bryant G. Ly wrote:
> >> This patch is dependent on:
> >> 'commit 25e78531268e ("ibmvscsis: Do not send aborted task response")'
> >> This patch cleans up some pointers after usage.
> >>
> >> Signed-off-by: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
> >> Reviewed-by: Michael Cyr <mike...@linux.vnet.ibm.com>
> >> Cc: <sta...@vger.kernel.org> # v4.8+
> >> ---
> >>   drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> > Applied, with a more verbose commit message.
> >
> > Thanks for splitting this out into an incremental patch.
> >
> No problem, did you end up forgetting it once you reverted Bart's 
> VERIFY/WRITE VERIFY patch?
> 
> It seems to be missing from your for-next now.
> 

Yep, going to include it post -rc1 with your latest patch.

Re: [PATCH v2] ibmvscsis: Fix the incorrect req_lim_delta

[Nicholas A. Bellinger]

On Wed, 2017-05-10 at 14:35 -0500, Bryant G. Ly wrote:
> The current code is not correctly calculating the req_lim_delta.
> 
> We want to make sure vscsi->credit is always incremented when
> we do not send a response for the scsi op. Thus for the case where
> there is a successfully aborted task we need to make sure the
> vscsi->credit is incremented.
> 
> v2 - Moves the original location of the vscsi->credit increment
> to a better spot. Since if we increment credit, the next command
> we send back will have increased req_lim_delta. But we probably
> shouldn't be doing that until the aborted cmd is actually released.
> Otherwise the client will think that it can send a new command, and
> we could find ourselves short of command elements. Not likely, but could
> happen.
> 
> This patch depends on both:
> commit 25e78531268e ("ibmvscsis: Do not send aborted task response")
> commit 38b2788edbd6 ("ibmvscsis: Clear left-over abort_cmd pointers")
> 
> Signed-off-by: Bryant G. Ly 
> Reviewed-by: Michael Cyr 
> Cc:  # v4.8+
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 24 
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index ee64241..abf6026 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1791,6 +1791,25 @@ static void ibmvscsis_send_messages(struct scsi_info 
> *vscsi)
>   list_del(>list);
>   ibmvscsis_free_cmd_resources(vscsi,
>cmd);
> + /*
> +  * With a successfully aborted op
> +  * through LIO we want to increment the
> +  * the vscsi credit so that when we dont
> +  * send a rsp to the original scsi abort
> +  * op (h_send_crq), but the tm rsp to
> +  * the abort is sent, the credit is
> +  * correctly sent with the abort tm rsp.
> +  * We would need 1 for the abort tm rsp
> +  * and 1 credit for the aborted scsi op.
> +  * Thus we need to increment here.
> +  * Also we want to increment the credit
> +  * here because we want to make sure
> +  * cmd is actually released first
> +  * otherwise the client will think it
> +  * it can send a new cmd, and we could
> +  * find ourselves short of cmd elements.
> +  */
> + vscsi->credit += 1;
>   } else {
>   iue = cmd->iue;
>  
> @@ -2965,10 +2984,7 @@ static long srp_build_response(struct scsi_info *vscsi,
>  
>   rsp->opcode = SRP_RSP;
>  
> - if (vscsi->credit > 0 && vscsi->state == SRP_PROCESSING)
> - rsp->req_lim_delta = cpu_to_be32(vscsi->credit);
> - else
> - rsp->req_lim_delta = cpu_to_be32(1 + vscsi->credit);
> + rsp->req_lim_delta = cpu_to_be32(1 + vscsi->credit);
>   rsp->tag = cmd->rsp.tag;
>   rsp->flags = 0;
>  

Thanks Bryant.  Will apply post -rc1.

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

[Nicholas A. Bellinger]

On Thu, 2017-05-11 at 08:26 +0200, h...@lst.de wrote:
> On Wed, May 10, 2017 at 09:50:35PM -0700, Nicholas A. Bellinger wrote:
> > 1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
> > support up to IBLOCK, in order to maintain SCSI target feature
> > compatibility.
> 
> No way.  If you want to zero use REQ_OP_WRITE_ZEROES..

Yes, I understand that part and it's what the earlier conversion of
IBLOCK to use blkdev_issue_zeroout() already does.

Once the blkdev_issue_zeroout() conversion is in place then LBPRZ can
always be set to one for IBLOCK using blkdev_issue_zeroout().

The point is this is not in -rc1, which as-is breaks LBPRZ compat for a
release.

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

[Nicholas A. Bellinger]

On Wed, 2017-05-10 at 16:06 +0200, h...@lst.de wrote:
> On Mon, May 08, 2017 at 11:46:14PM -0700, Nicholas A. Bellinger wrote:
> > That said, simply propagating up q->limits.max_write_zeroes_sectors as
> > dev_attrib->unmap_zeroes_data following existing code still looks like
> > the right thing to do.
> 
> It is not.  Martin has decoupled write same/zeroes support from discard
> support.  Any device will claim to support it initially, and we'll
> only clear the flag if a Write Same command fails.
> 
> So even if LBPRZ is not set you can trivially get into a situation
> where discard is supported through UNMAP, and you'll incorrectly
> set LBPRZ and will cause data corruption.

In that case, there are two choices.

1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
support up to IBLOCK, in order to maintain SCSI target feature
compatibility.

2) Or drop the LBPRZ bit usage for IBLOCK all-together.

Since I happen happen to support a block driver that has 'real LBPRZ'
support for all discards, I'd prefer the latter so this doesn't have to
be carried out-of-tree.

So what are the options for this in post v4.12..?

Re: [PATCH v1] ibmvscsis: Fix cleaning up pointers

[Nicholas A. Bellinger]

On Tue, 2017-05-09 at 11:50 -0500, Bryant G. Ly wrote:
> This patch is dependent on:
> 'commit 25e78531268e ("ibmvscsis: Do not send aborted task response")'
> This patch cleans up some pointers after usage.
> 
> Signed-off-by: Bryant G. Ly 
> Reviewed-by: Michael Cyr 
> Cc:  # v4.8+
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, with a more verbose commit message.

Thanks for splitting this out into an incremental patch.

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

[Nicholas A. Bellinger]

On Sun, 2017-05-07 at 11:22 +0200, h...@lst.de wrote:
> On Tue, May 02, 2017 at 08:33:15PM -0700, Nicholas A. Bellinger wrote:
> > The larger target/iblock conversion patch looks like post v4.12 material
> > at this point, so to avoid breakage wrt to existing LBPRZ behavior, I'll
> > plan to push the following patch post -rc1.
> 
> I don't think this is safe.  If you want to do the aboe you also
> need to ensure ->execute_unmap always zeroes the data.  For actual
> files in the file backend we should be all fine, but for the block
> device case [1] and iblock we'd need to use blkdev_issue_zeroout
> instead of blkdev_issue_discard when unmap_zeroes_data is set.
> 
> [1] which btw already seems broken as it doesn't invalidate cached
> data when issuing a discard.

Mmm, for [1] that would appear to be true, but after a deeper look at
existing code I don't think this is the case.

The reason being is target backend attributes emulate_tpu and
emulate_tpws are strictly user configurable, and aren't automatically
set based upon the underlying IBLOCK block_device support for either
one.

According to pre v4.12-rc1 code, q->limits.discard_zeroes_data was only
enabled by drivers/scsi/sd.c:sd_config_discard() for
sdkp->provisioning_mode WRITE_SAME with LBPRZ = 1 or explicit ZERO, and
for NVME for devices that supported NVME_QUIRK_DISCARD_ZEROES.  Eg: Only
real DISCARD + ZERO support.

In your changes to v4.12-rc1, this logic to signal real DISCARD + zero
support for SCSI and NVMe via q->limits.max_write_zeroes_sectors has not
changed..

So AFAICT, regardless if the user sets emulate_tpu or emulate_tpws for a
IBLOCK backend, SCSI host code will have to choose sdkp->zeroing_mode
WRITE_SAME with LBPRZ or explicit ZERO, and NVMe host code will have to
chose a ctrl NVME_QUIRK_DEALLOCATE_ZEROES before
q->limits.max_write_zeroes_sectors != 0 is propagated up to target code,
and LBPRZ = 1 is signaled via READ_CAPACITY_16 and EVPD = 0xb2.

That said, simply propagating up q->limits.max_write_zeroes_sectors as
dev_attrib->unmap_zeroes_data following existing code still looks like
the right thing to do.

Re: [PATCH v4] ibmvscsis: Do not send aborted task response

[Nicholas A. Bellinger]

Hi Bryant,

Given running we're almost out of time for -rc1, I'd like to avoid
having to rebase the handful of patches that are atop the -v3 that was
applied to target-pending/for-next over the weekend...

So if you'd be so kind, please post an incremental patch atop -v3, and
I'll apply that instead.

On Mon, 2017-05-08 at 17:07 -0500, Bryant G. Ly wrote:
> The driver is sending a response to the actual scsi op that was
> aborted by an abort task TM, while LIO is sending a response to
> the abort task TM.
> 
> ibmvscsis_tgt does not send the response to the client until
> release_cmd time. The reason for this was because if we did it
> at queue_status time, then the client would be free to reuse the
> tag for that command, but we're still using the tag until the
> command is released at release_cmd time, so we chose to delay
> sending the response until then. That then caused this issue, because
> release_cmd is always called, even if queue_status is not.
> 
> SCSI spec says that the initiator that sends the abort task
> TM NEVER gets a response to the aborted op and with the current
> code it will send a response. Thus this fix will remove that response
> if the CMD_T_ABORTED && !CMD_T_TAS.
> 
> Another case with a small timing window is the case where if LIO sends a
> TMR_DOES_NOT_EXIST, and the release_cmd callback is called for the TMR Abort
> cmd before the release_cmd for the (attemped) aborted cmd, then we need to
> ensure that we send the response for the (attempted) abort cmd to the client
> before we send the response for the TMR Abort cmd.
> 
> Forgot to reset the cmd->abort_cmd pointer after finding it in send_messages.
> 
> Cc:  # v4.8+
> Signed-off-by: Bryant G. Ly 
> Signed-off-by: Michael Cyr 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 117 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |   2 +
>  2 files changed, 94 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 0f80779..ee64241 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1170,6 +1170,9 @@ static struct ibmvscsis_cmd 
> *ibmvscsis_get_free_cmd(struct scsi_info *vscsi)
>   cmd = list_first_entry_or_null(>free_cmd,
>  struct ibmvscsis_cmd, list);
>   if (cmd) {
> + if (cmd->abort_cmd)
> + cmd->abort_cmd = NULL;
> + cmd->flags &= ~(DELAY_SEND);
>   list_del(>list);
>   cmd->iue = iue;
>   cmd->type = UNSET_TYPE;
> @@ -1749,45 +1752,80 @@ static void srp_snd_msg_failed(struct scsi_info 
> *vscsi, long rc)
>  static void ibmvscsis_send_messages(struct scsi_info *vscsi)
>  {
>   u64 msg_hi = 0;
> - /* note do not attmempt to access the IU_data_ptr with this pointer
> + /* note do not attempt to access the IU_data_ptr with this pointer
>* it is not valid
>*/
>   struct viosrp_crq *crq = (struct viosrp_crq *)_hi;
>   struct ibmvscsis_cmd *cmd, *nxt;
>   struct iu_entry *iue;
>   long rc = ADAPT_SUCCESS;
> + bool retry = false;
>  
>   if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
> - list_for_each_entry_safe(cmd, nxt, >waiting_rsp, list) {
> - iue = cmd->iue;
> + do {
> + retry = false;
> + list_for_each_entry_safe(cmd, nxt, >waiting_rsp,
> +  list) {
> + /*
> +  * Check to make sure abort cmd gets processed
> +  * prior to the abort tmr cmd
> +  */
> + if (cmd->flags & DELAY_SEND)
> + continue;
>  
> - crq->valid = VALID_CMD_RESP_EL;
> - crq->format = cmd->rsp.format;
> + if (cmd->abort_cmd) {
> + retry = true;
> + cmd->abort_cmd->flags &= ~(DELAY_SEND);
> + cmd->abort_cmd = NULL;
> + }
>  
> - if (cmd->flags & CMD_FAST_FAIL)
> - crq->status = VIOSRP_ADAPTER_FAIL;
> + /*
> +  * If CMD_T_ABORTED w/o CMD_T_TAS scenarios and
> +  * the case where LIO issued a
> +  * ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST
> +  * case then we dont send a response, since it
> +  * was already done.
> +  */
> 

Re: [PATCH v3] ibmvscsis: Do not send aborted task response

[Nicholas A. Bellinger]

On Fri, 2017-05-05 at 14:17 -0500, Bryant G. Ly wrote:
> The driver is sending a response to the actual scsi op that was
> aborted by an abort task TM, while LIO is sending a response to
> the abort task TM.
> 
> ibmvscsis_tgt does not send the response to the client until
> release_cmd time. The reason for this was because if we did it
> at queue_status time, then the client would be free to reuse the
> tag for that command, but we're still using the tag until the
> command is released at release_cmd time, so we chose to delay
> sending the response until then. That then caused this issue, because
> release_cmd is always called, even if queue_status is not.
> 
> SCSI spec says that the initiator that sends the abort task
> TM NEVER gets a response to the aborted op and with the current
> code it will send a response. Thus this fix will remove that response
> if the CMD_T_ABORTED && !CMD_T_TAS.
> 
> Another case with a small timing window is the case where if LIO sends a
> TMR_DOES_NOT_EXIST, and the release_cmd callback is called for the TMR Abort
> cmd before the release_cmd for the (attemped) aborted cmd, then we need to
> ensure that we send the response for the (attempted) abort cmd to the client
> before we send the response for the TMR Abort cmd.
> 
> Cc:  # v4.8+
> Signed-off-by: Bryant G. Ly 
> Signed-off-by: Michael Cyr 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 114 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |   2 +
>  2 files changed, 91 insertions(+), 25 deletions(-)
> 

Applied.

Thanks Bryant + Michael.