[PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

For VERIFY and WRITE AND VERIFY commands the size of the SCSI
Data-Out buffer can differ from the size of the data area on the
storage medium that is affected by the command. Make sure that
the Data-Out buffer size is computed correctly if the BYTCHK
field in the CDB is zero. This patch reverts commit 984a9d4c40be
and thereby restores commit 0e2eb7d12eaa. Additionally,
sbc_parse_cdb() is modified such that the data buffer size is
computed correctly for the affected commands if BYTCHK == 0.
This patch is the combination of two patches that got positive
reviews.

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

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 4316f7b65fb7..51489d96cb31 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -831,12 +831,67 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
 	return 0;
 }
 
+/**
+ * sbc_parse_verify - parse VERIFY, VERIFY_16 and WRITE VERIFY commands
+ * @cmd:     (in)  structure that describes the SCSI command to be parsed.
+ * @sectors: (out) Number of logical blocks on the storage medium that will be
+ *           affected by the SCSI command.
+ * @bufflen: (out) Expected length of the SCSI Data-Out buffer.
+ */
+static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
+				       u32 *bufflen)
+{
+	struct se_device *dev = cmd->se_dev;
+	u8 *cdb = cmd->t_task_cdb;
+	u8 bytchk = (cdb[1] >> 1) & 3;
+	sense_reason_t ret;
+
+	switch (cdb[0]) {
+	case VERIFY:
+	case WRITE_VERIFY:
+		*sectors = transport_get_sectors_10(cdb);
+		cmd->t_task_lba = transport_lba_32(cdb);
+		break;
+	case VERIFY_16:
+	case WRITE_VERIFY_16:
+		*sectors = transport_get_sectors_16(cdb);
+		cmd->t_task_lba = transport_lba_64(cdb);
+		break;
+	default:
+		WARN_ON_ONCE(true);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (sbc_check_dpofua(dev, cmd, cdb))
+		return TCM_INVALID_CDB_FIELD;
+
+	ret = sbc_check_prot(dev, cmd, cdb, *sectors, true);
+	if (ret)
+		return ret;
+
+	switch (bytchk) {
+	case 0:
+		*bufflen = 0;
+		break;
+	case 1:
+		*bufflen = sbc_get_size(cmd, *sectors);
+		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
+		break;
+	default:
+		pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n",
+		       bytchk, cdb[0]);
+		return TCM_INVALID_CDB_FIELD;
+	}
+	return TCM_NO_SENSE;
+}
+
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
+	enum { INVALID_SIZE = 1 };
 	struct se_device *dev = cmd->se_dev;
 	unsigned char *cdb = cmd->t_task_cdb;
-	unsigned int size;
+	unsigned int size = INVALID_SIZE;
 	u32 sectors = 0;
 	sense_reason_t ret;
 
@@ -898,7 +953,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
-	case WRITE_VERIFY:
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
@@ -912,6 +966,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
+	case WRITE_VERIFY:
+	case WRITE_VERIFY_16:
+		ret = sbc_parse_verify(cmd, &sectors, &size);
+		if (ret)
+			return ret;
+		cmd->execute_cmd = sbc_execute_rw;
+		goto check_lba;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
@@ -927,7 +988,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_16:
-	case WRITE_VERIFY_16:
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
@@ -1110,14 +1170,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case VERIFY:
 	case VERIFY_16:
-		size = 0;
-		if (cdb[0] == VERIFY) {
-			sectors = transport_get_sectors_10(cdb);
-			cmd->t_task_lba = transport_lba_32(cdb);
-		} else {
-			sectors = transport_get_sectors_16(cdb);
-			cmd->t_task_lba = transport_lba_64(cdb);
-		}
+		ret = sbc_parse_verify(cmd, &sectors, &size);
+		if (ret)
+			return ret;
 		cmd->execute_cmd = sbc_emulate_noop;
 		goto check_lba;
 	case REZERO_UNIT:
@@ -1158,7 +1213,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			return TCM_ADDRESS_OUT_OF_RANGE;
 		}
 
-		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
+		if (size == INVALID_SIZE)
 			size = sbc_get_size(cmd, sectors);
 	}
 
-- 
2.12.2

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> Data-Out buffer can differ from the size of the data area on the
> storage medium that is affected by the command. Make sure that
> the Data-Out buffer size is computed correctly if the BYTCHK
> field in the CDB is zero. This patch reverts commit 984a9d4c40be
> and thereby restores commit 0e2eb7d12eaa. Additionally,
> sbc_parse_cdb() is modified such that the data buffer size is
> computed correctly for the affected commands if BYTCHK == 0.
> This patch is the combination of two patches that got positive
> reviews.
> 
> References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"")
> References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 67 insertions(+), 12 deletions(-)
> 

This patch ignored the review comments from the last round:

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

Until these are addressed as requested, dropping this patch for now.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > Data-Out buffer can differ from the size of the data area on the
> > storage medium that is affected by the command. Make sure that
> > the Data-Out buffer size is computed correctly if the BYTCHK
> > field in the CDB is zero. This patch reverts commit 984a9d4c40be
> > and thereby restores commit 0e2eb7d12eaa. Additionally,
> > sbc_parse_cdb() is modified such that the data buffer size is
> > computed correctly for the affected commands if BYTCHK == 0.
> > This patch is the combination of two patches that got positive
> > reviews.
> > 
> > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"")
> > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Andy Grover <agrover@redhat.com>
> > Cc: David Disseldorp <ddiss@suse.de>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 67 insertions(+), 12 deletions(-)
> > 
> 
> This patch ignored the review comments from the last round:
> 
> http://www.spinics.net/lists/target-devel/msg15306.html
> http://www.spinics.net/lists/target-devel/msg15327.html
> 
> Until these are addressed as requested, dropping this patch for now.

Hello Nic,

In this patch series I have addressed all comments that made sense to me. Sorry
if you feel offended because I had not addressed the two comments you referred to
above. The reason I had not addressed these comments is because these comments
are wrong in my opinion. Hence, please reconsider this patch.

Bart.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote:
> On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > > Data-Out buffer can differ from the size of the data area on the
> > > storage medium that is affected by the command. Make sure that
> > > the Data-Out buffer size is computed correctly if the BYTCHK
> > > field in the CDB is zero. This patch reverts commit 984a9d4c40be
> > > and thereby restores commit 0e2eb7d12eaa. Additionally,
> > > sbc_parse_cdb() is modified such that the data buffer size is
> > > computed correctly for the affected commands if BYTCHK == 0.
> > > This patch is the combination of two patches that got positive
> > > reviews.
> > > 
> > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"")
> > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > Cc: Hannes Reinecke <hare@suse.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Andy Grover <agrover@redhat.com>
> > > Cc: David Disseldorp <ddiss@suse.de>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > >  drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 67 insertions(+), 12 deletions(-)
> > > 
> > 
> > This patch ignored the review comments from the last round:
> > 
> > http://www.spinics.net/lists/target-devel/msg15306.html
> > http://www.spinics.net/lists/target-devel/msg15327.html
> > 
> > Until these are addressed as requested, dropping this patch for now.
> 
> Hello Nic,
> 
> In this patch series I have addressed all comments that made sense to me. Sorry
> if you feel offended because I had not addressed the two comments you referred to
> above. The reason I had not addressed these comments is because these comments
> are wrong in my opinion. Hence, please reconsider this patch.

Nope.  Here are the details again. 

First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases,
and only sets it for BYTCHK=0.

Yes, I understand the spec says hosts are not supposed to send a payload
when BYTCHK=0, but that doesn't stop some from trying.

Any CDB that can potentially allocate SGLS via target_alloc_sgl() must
set this flag.  No other CDBs set SCF_SCSI_DATA_CDB based on bits in the
CDB, and *_VERIFY is no exception.

Secondly, the force setting of size in sbc_parse_verify(), instead of
what was actually received over the write is totally wrong.  Like I said
before, the size in sbc_parse_cdb() is what's extracted from the CDB
transfer length, and not what the spec says the correct size should be.

Tthat said, I already reverted this patch once because you didn't want
to make these very simple changes, so I'll not be merging it until the
changes are made.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Nicholas A. Bellinger]

On Fri, 2017-06-02 at 22:32 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote:
> > On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote:
> > > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > > > Data-Out buffer can differ from the size of the data area on the
> > > > storage medium that is affected by the command. Make sure that
> > > > the Data-Out buffer size is computed correctly if the BYTCHK
> > > > field in the CDB is zero. This patch reverts commit 984a9d4c40be
> > > > and thereby restores commit 0e2eb7d12eaa. Additionally,
> > > > sbc_parse_cdb() is modified such that the data buffer size is
> > > > computed correctly for the affected commands if BYTCHK == 0.
> > > > This patch is the combination of two patches that got positive
> > > > reviews.
> > > > 
> > > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"")
> > > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > > Cc: Hannes Reinecke <hare@suse.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Andy Grover <agrover@redhat.com>
> > > > Cc: David Disseldorp <ddiss@suse.de>
> > > > Cc: <stable@vger.kernel.org>
> > > > ---
> > > >  drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 67 insertions(+), 12 deletions(-)
> > > > 
> > > 
> > > This patch ignored the review comments from the last round:
> > > 
> > > http://www.spinics.net/lists/target-devel/msg15306.html
> > > http://www.spinics.net/lists/target-devel/msg15327.html
> > > 
> > > Until these are addressed as requested, dropping this patch for now.
> > 
> > Hello Nic,
> > 
> > In this patch series I have addressed all comments that made sense to me. Sorry
> > if you feel offended because I had not addressed the two comments you referred to
> > above. The reason I had not addressed these comments is because these comments
> > are wrong in my opinion. Hence, please reconsider this patch.
> 
> Nope.  Here are the details again. 
> 
> First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases,
> and only sets it for BYTCHK=0.

or rather, and only sets it (SCF_SCSI_DATA_CDB) for BYTCHK=1.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Fri, 2017-06-02 at 22:32 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote:
> > In this patch series I have addressed all comments that made sense to me. Sorry
> > if you feel offended because I had not addressed the two comments you referred to
> > above. The reason I had not addressed these comments is because these comments
> > are wrong in my opinion. Hence, please reconsider this patch.
> 
> Nope.  Here are the details again. 
> 
> First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases,
> and only sets it for BYTCHK=0.
> 
> Yes, I understand the spec says hosts are not supposed to send a payload
> when BYTCHK=0, but that doesn't stop some from trying.
> 
> Any CDB that can potentially allocate SGLS via target_alloc_sgl() must
> set this flag.  No other CDBs set SCF_SCSI_DATA_CDB based on bits in the
> CDB, and *_VERIFY is no exception.

A quote from the SBC-4 section about VERIFY(10): "If the byte check (BYTCHK)
field is set to 00b, then no Data-Out Buffer transfer shall occur". In other
words, if a VERIFY or WRITE VERIFY command is received with BYTCHK=0,
transferring the Data-Out buffer is not only superfluous it is also against
the SCSI specs. Today target_cmd_size_check() terminates SCSI commands for
which the size of the Data-Out buffer exceeds the expected size with
TCM_INVALID_CDB_FIELD so the data transfer doesn't happen anyway. Hence it
is not necessary to allocate an SGL with target_alloc_sgl() if BYTCHK=0.

> Secondly, the force setting of size in sbc_parse_verify(), instead of
> what was actually received over the write is totally wrong.  Like I said
> before, the size in sbc_parse_cdb() is what's extracted from the CDB
> transfer length, and not what the spec says the correct size should be.

Please take the SCSI specs seriously instead of ignoring the SCSI specs. I
think for VERIFY and WRITE VERIFY with BYTCHK=0, the size extracted from the
CDB should be zero bytes.

What's needed in my opinion to make VERIFY and WRITE VERIFY processing
compliant with the SCSI specs is the following:
- Patch 04/33 from this series that fixes the parsing of these commands.
- Patch 25/33 from this series that fixes handling of too large Data-Out
  buffers for the iSCSI target driver (the qla2xxx and ib_srpt target drivers
  already handle this case correctly).
- Patch 30/33 from this series that makes target_cmd_size_check() send the
  correct sense code to the initiator system for too large Data-Out buffers.

Bart.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[David Butterfield]

On Mon, Jun 5, 2017 at 10:49 AM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> A quote from the SBC-4 section about VERIFY(10): "If the byte check (BYTCHK)
> field is set to 00b, then no Data-Out Buffer transfer shall occur". In other
> words, if a VERIFY or WRITE VERIFY command is received with BYTCHK=0,
> transferring the Data-Out buffer is not only superfluous it is also against
> the SCSI specs.

I think this is true for VERIFY; but WRITE VERIFY is not so clear.  It
looks to me
like the SBC-4r13 description of WRITE VERIFY is broken.  It refers to VERIFY
for the definition of BYTCHK=01, but the definition in VERIFY refers to a CDB
field that does not exist in the WRITE VERIFY CDB (VERIFICATION LENGTH).

In WRITE VERIFY the most similar field is TRANSFER LENGTH.  I take that to
be intended as the amount of data to be written during the WRITE step.  Since
the spec refers to the nonexistent VERIFICATION LENGTH field for BYTCHK=01,
we have to guess that they intend the TRANSFER LENGTH to be used also as
the length of the VERIFY step, when BYTCHK=01.

In WRITE VERIFY with BYTCHK=00, I think we must consider the TRANSFER
LENGTH (usually nonzero) to apply to the WRITE step, no matter the setting
of BYTCHK.

Since the spec provides the BYTCHK field for WRITE VERIFY and refers to its
definition in VERIFY, I take that to mean the intention is that BYTCHK=00 be
treated as with VERIFY; in other words, the VERIFY step ignores TRANSFER
LENGTH (even though the WRITE step uses it), and the VERIFY step checks
the protection information based on the VRPROTECT field in the CDB
(WRPROTECT in the WRITE CDB), as described in the VERIFY section.

I think the spec is broken here, and this is my plausible
interpretation of the intent.

Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands

[Bart Van Assche]

On Mon, 2017-06-05 at 16:32 -0600, David Butterfield wrote:
> Since the spec provides the BYTCHK field for WRITE VERIFY and refers to its
> definition in VERIFY, I take that to mean the intention is that BYTCHK=00 be
> treated as with VERIFY; in other words, the VERIFY step ignores TRANSFER
> LENGTH (even though the WRITE step uses it), and the VERIFY step checks
> the protection information based on the VRPROTECT field in the CDB
> (WRPROTECT in the WRITE CDB), as described in the VERIFY section.
> 
> I think the spec is broken here, and this is my plausible
> interpretation of the intent.

Hello Dave,

That's a good point. As far as I know only AIX submits the WRITE AND VERIFY
command. So it would be useful to know what value AIX sets the BYTCHK field
to.

Bart.

[PATCH 08/33] target: Fix a deadlock between the XCOPY code and iSCSI session shutdown

[Bart Van Assche]

Move the code for parsing an XCOPY command from the context of
the iSCSI receiver thread to the context of the XCOPY workqueue.
Keep the simple XCOPY checks in the context of the iSCSI receiver
thread. Move the code for allocating and freeing struct xcopy_op
from the code that parses an XCOPY command to its caller.

This patch fixes the following deadlock:

======================================================
[ INFO: possible circular locking dependency detected ]
4.10.0-rc7-dbg+ #1 Not tainted
-------------------------------------------------------
rmdir/13321 is trying to acquire lock:
 (&sess->cmdsn_mutex){+.+.+.}, at: [<ffffffffa02cb47d>] iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]

but task is already holding lock:
 (&sb->s_type->i_mutex_key#14){++++++}, at: [<ffffffff811c6e20>] vfs_rmdir+0x50/0x140

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:
-> #1 (&sb->s_type->i_mutex_key#14){++++++}:
 lock_acquire+0x71/0x90
 down_write+0x3f/0x70
 configfs_depend_item+0x3a/0xb0 [configfs]
 target_depend_item+0x13/0x20 [target_core_mod]
 target_xcopy_locate_se_dev_e4+0xdd/0x1a0 [target_core_mod]
 target_do_xcopy+0x34b/0x970 [target_core_mod]
 __target_execute_cmd+0x22/0xa0 [target_core_mod]
 target_execute_cmd+0x233/0x2c0 [target_core_mod]
 iscsit_execute_cmd+0x208/0x270 [iscsi_target_mod]
 iscsit_sequence_cmd+0x10b/0x190 [iscsi_target_mod]
 iscsit_get_rx_pdu+0x37d/0xcd0 [iscsi_target_mod]
 iscsi_target_rx_thread+0x6e/0xa0 [iscsi_target_mod]
 kthread+0x102/0x140
 ret_from_fork+0x31/0x40

-> #0 (&sess->cmdsn_mutex){+.+.+.}:
 __lock_acquire+0x10e6/0x1260
 lock_acquire+0x71/0x90
 mutex_lock_nested+0x5f/0x670
 iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]
 iscsit_close_session+0xac/0x200 [iscsi_target_mod]
 lio_tpg_close_session+0x9f/0xb0 [iscsi_target_mod]
 target_shutdown_sessions+0xc3/0xd0 [target_core_mod]
 core_tpg_del_initiator_node_acl+0x91/0x140 [target_core_mod]
 target_fabric_nacl_base_release+0x20/0x30 [target_core_mod]
 config_item_release+0x5a/0xc0 [configfs]
 config_item_put+0x1d/0x1f [configfs]
 configfs_rmdir+0x1a6/0x300 [configfs]
 vfs_rmdir+0xb7/0x140
 do_rmdir+0x1f4/0x200
 SyS_rmdir+0x11/0x20
 entry_SYSCALL_64_fastpath+0x23/0xc6

other info that might help us debug this:

 Possible unsafe locking scenario:
       CPU0                    CPU1
       ----                    ----
  lock(&sb->s_type->i_mutex_key#14);
                               lock(&sess->cmdsn_mutex);
                               lock(&sb->s_type->i_mutex_key#14);
  lock(&sess->cmdsn_mutex);

 *** DEADLOCK ***

3 locks held by rmdir/13321:
 #0:  (sb_writers#10){.+.+.+}, at: [<ffffffff811e1aff>] mnt_want_write+0x1f/0x50
 #1:  (&default_group_class[depth - 1]#2/1){+.+.+.}, at: [<ffffffff811cc8ce>] do_rmdir+0x15e/0x200
 #2:  (&sb->s_type->i_mutex_key#14){++++++}, at: [<ffffffff811c6e20>] vfs_rmdir+0x50/0x140

stack backtrace:
CPU: 2 PID: 13321 Comm: rmdir Not tainted 4.10.0-rc7-dbg+ #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
Call Trace:
 dump_stack+0x86/0xc3
 print_circular_bug+0x1c7/0x220
 __lock_acquire+0x10e6/0x1260
 lock_acquire+0x71/0x90
 mutex_lock_nested+0x5f/0x670
 iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]
 iscsit_close_session+0xac/0x200 [iscsi_target_mod]
 lio_tpg_close_session+0x9f/0xb0 [iscsi_target_mod]
 target_shutdown_sessions+0xc3/0xd0 [target_core_mod]
 core_tpg_del_initiator_node_acl+0x91/0x140 [target_core_mod]
 target_fabric_nacl_base_release+0x20/0x30 [target_core_mod]
 config_item_release+0x5a/0xc0 [configfs]
 config_item_put+0x1d/0x1f [configfs]
 configfs_rmdir+0x1a6/0x300 [configfs]
 vfs_rmdir+0xb7/0x140
 do_rmdir+0x1f4/0x200
 SyS_rmdir+0x11/0x20
 entry_SYSCALL_64_fastpath+0x23/0xc6

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

diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index f12cf0c12531..56738a41e346 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -40,6 +40,8 @@
 
 static struct workqueue_struct *xcopy_wq = NULL;
 
+static sense_reason_t target_parse_xcopy_cmd(struct xcopy_op *xop);
+
 static int target_xcopy_gen_naa_ieee(struct se_device *dev, unsigned char *buf)
 {
 	int off = 0;
@@ -779,13 +781,24 @@ static int target_xcopy_write_destination(
 static void target_xcopy_do_work(struct work_struct *work)
 {
 	struct xcopy_op *xop = container_of(work, struct xcopy_op, xop_work);
-	struct se_device *src_dev = xop->src_dev, *dst_dev = xop->dst_dev;
 	struct se_cmd *ec_cmd = xop->xop_se_cmd;
-	sector_t src_lba = xop->src_lba, dst_lba = xop->dst_lba, end_lba;
+	struct se_device *src_dev, *dst_dev;
+	sector_t src_lba, dst_lba, end_lba;
 	unsigned int max_sectors;
-	int rc;
-	unsigned short nolb = xop->nolb, cur_nolb, max_nolb, copied_nolb = 0;
+	int rc = 0;
+	unsigned short nolb, cur_nolb, max_nolb, copied_nolb = 0;
+
+	if (target_parse_xcopy_cmd(xop) != TCM_NO_SENSE)
+		goto err_free;
 
+	if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev))
+		goto err_free;
+
+	src_dev = xop->src_dev;
+	dst_dev = xop->dst_dev;
+	src_lba = xop->src_lba;
+	dst_lba = xop->dst_lba;
+	nolb = xop->nolb;
 	end_lba = src_lba + nolb;
 	/*
 	 * Break up XCOPY I/O into hw_max_sectors sized I/O based on the
@@ -853,6 +866,8 @@ static void target_xcopy_do_work(struct work_struct *work)
 
 out:
 	xcopy_pt_undepend_remotedev(xop);
+
+err_free:
 	kfree(xop);
 	/*
 	 * Don't override an error scsi status if it has already been set
@@ -865,48 +880,22 @@ static void target_xcopy_do_work(struct work_struct *work)
 	target_complete_cmd(ec_cmd, ec_cmd->scsi_status);
 }
 
-sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
+/*
+ * Returns TCM_NO_SENSE upon success or a sense code != TCM_NO_SENSE if parsing
+ * fails.
+ */
+static sense_reason_t target_parse_xcopy_cmd(struct xcopy_op *xop)
 {
-	struct se_device *dev = se_cmd->se_dev;
-	struct xcopy_op *xop = NULL;
+	struct se_cmd *se_cmd = xop->xop_se_cmd;
 	unsigned char *p = NULL, *seg_desc;
-	unsigned int list_id, list_id_usage, sdll, inline_dl, sa;
+	unsigned int list_id, list_id_usage, sdll, inline_dl;
 	sense_reason_t ret = TCM_INVALID_PARAMETER_LIST;
 	int rc;
 	unsigned short tdll;
 
-	if (!dev->dev_attrib.emulate_3pc) {
-		pr_err("EXTENDED_COPY operation explicitly disabled\n");
-		return TCM_UNSUPPORTED_SCSI_OPCODE;
-	}
-
-	sa = se_cmd->t_task_cdb[1] & 0x1f;
-	if (sa != 0x00) {
-		pr_err("EXTENDED_COPY(LID4) not supported\n");
-		return TCM_UNSUPPORTED_SCSI_OPCODE;
-	}
-
-	if (se_cmd->data_length == 0) {
-		target_complete_cmd(se_cmd, SAM_STAT_GOOD);
-		return TCM_NO_SENSE;
-	}
-	if (se_cmd->data_length < XCOPY_HDR_LEN) {
-		pr_err("XCOPY parameter truncation: length %u < hdr_len %u\n",
-				se_cmd->data_length, XCOPY_HDR_LEN);
-		return TCM_PARAMETER_LIST_LENGTH_ERROR;
-	}
-
-	xop = kzalloc(sizeof(struct xcopy_op), GFP_KERNEL);
-	if (!xop) {
-		pr_err("Unable to allocate xcopy_op\n");
-		return TCM_OUT_OF_RESOURCES;
-	}
-	xop->xop_se_cmd = se_cmd;
-
 	p = transport_kmap_data_sg(se_cmd);
 	if (!p) {
 		pr_err("transport_kmap_data_sg() failed in target_do_xcopy\n");
-		kfree(xop);
 		return TCM_OUT_OF_RESOURCES;
 	}
 
@@ -975,18 +964,57 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
 	pr_debug("XCOPY: Processed %d target descriptors, length: %u\n", rc,
 				rc * XCOPY_TARGET_DESC_LEN);
 	transport_kunmap_data_sg(se_cmd);
-
-	INIT_WORK(&xop->xop_work, target_xcopy_do_work);
-	queue_work(xcopy_wq, &xop->xop_work);
 	return TCM_NO_SENSE;
 
 out:
 	if (p)
 		transport_kunmap_data_sg(se_cmd);
-	kfree(xop);
 	return ret;
 }
 
+sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
+{
+	struct se_device *dev = se_cmd->se_dev;
+	struct xcopy_op *xop;
+	unsigned int sa;
+
+	if (!dev->dev_attrib.emulate_3pc) {
+		pr_err("EXTENDED_COPY operation explicitly disabled\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	sa = se_cmd->t_task_cdb[1] & 0x1f;
+	if (sa != 0x00) {
+		pr_err("EXTENDED_COPY(LID4) not supported\n");
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (se_cmd->data_length == 0) {
+		target_complete_cmd(se_cmd, SAM_STAT_GOOD);
+		return TCM_NO_SENSE;
+	}
+	if (se_cmd->data_length < XCOPY_HDR_LEN) {
+		pr_err("XCOPY parameter truncation: length %u < hdr_len %u\n",
+				se_cmd->data_length, XCOPY_HDR_LEN);
+		return TCM_PARAMETER_LIST_LENGTH_ERROR;
+	}
+
+	xop = kzalloc(sizeof(struct xcopy_op), GFP_KERNEL);
+	if (!xop)
+		goto err;
+	xop->xop_se_cmd = se_cmd;
+	INIT_WORK(&xop->xop_work, target_xcopy_do_work);
+	if (WARN_ON_ONCE(!queue_work(xcopy_wq, &xop->xop_work)))
+		goto free;
+	return TCM_NO_SENSE;
+
+free:
+	kfree(xop);
+
+err:
+	return TCM_OUT_OF_RESOURCES;
+}
+
 static sense_reason_t target_rcr_operating_parameters(struct se_cmd *se_cmd)
 {
 	unsigned char *p;
-- 
2.12.2

Re: [PATCH 08/33] target: Fix a deadlock between the XCOPY code and iSCSI session shutdown

[Nicholas A. Bellinger]

On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> Move the code for parsing an XCOPY command from the context of
> the iSCSI receiver thread to the context of the XCOPY workqueue.
> Keep the simple XCOPY checks in the context of the iSCSI receiver
> thread. Move the code for allocating and freeing struct xcopy_op
> from the code that parses an XCOPY command to its caller.
> 
> This patch fixes the following deadlock:
> 
> ======================================================
> [ INFO: possible circular locking dependency detected ]
> 4.10.0-rc7-dbg+ #1 Not tainted
> -------------------------------------------------------
> rmdir/13321 is trying to acquire lock:
>  (&sess->cmdsn_mutex){+.+.+.}, at: [<ffffffffa02cb47d>] iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]
> 
> but task is already holding lock:
>  (&sb->s_type->i_mutex_key#14){++++++}, at: [<ffffffff811c6e20>] vfs_rmdir+0x50/0x140
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> -> #1 (&sb->s_type->i_mutex_key#14){++++++}:
>  lock_acquire+0x71/0x90
>  down_write+0x3f/0x70
>  configfs_depend_item+0x3a/0xb0 [configfs]
>  target_depend_item+0x13/0x20 [target_core_mod]
>  target_xcopy_locate_se_dev_e4+0xdd/0x1a0 [target_core_mod]
>  target_do_xcopy+0x34b/0x970 [target_core_mod]
>  __target_execute_cmd+0x22/0xa0 [target_core_mod]
>  target_execute_cmd+0x233/0x2c0 [target_core_mod]
>  iscsit_execute_cmd+0x208/0x270 [iscsi_target_mod]
>  iscsit_sequence_cmd+0x10b/0x190 [iscsi_target_mod]
>  iscsit_get_rx_pdu+0x37d/0xcd0 [iscsi_target_mod]
>  iscsi_target_rx_thread+0x6e/0xa0 [iscsi_target_mod]
>  kthread+0x102/0x140
>  ret_from_fork+0x31/0x40
> 
> -> #0 (&sess->cmdsn_mutex){+.+.+.}:
>  __lock_acquire+0x10e6/0x1260
>  lock_acquire+0x71/0x90
>  mutex_lock_nested+0x5f/0x670
>  iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]
>  iscsit_close_session+0xac/0x200 [iscsi_target_mod]
>  lio_tpg_close_session+0x9f/0xb0 [iscsi_target_mod]
>  target_shutdown_sessions+0xc3/0xd0 [target_core_mod]
>  core_tpg_del_initiator_node_acl+0x91/0x140 [target_core_mod]
>  target_fabric_nacl_base_release+0x20/0x30 [target_core_mod]
>  config_item_release+0x5a/0xc0 [configfs]
>  config_item_put+0x1d/0x1f [configfs]
>  configfs_rmdir+0x1a6/0x300 [configfs]
>  vfs_rmdir+0xb7/0x140
>  do_rmdir+0x1f4/0x200
>  SyS_rmdir+0x11/0x20
>  entry_SYSCALL_64_fastpath+0x23/0xc6
> 
> other info that might help us debug this:
> 
>  Possible unsafe locking scenario:
>        CPU0                    CPU1
>        ----                    ----
>   lock(&sb->s_type->i_mutex_key#14);
>                                lock(&sess->cmdsn_mutex);
>                                lock(&sb->s_type->i_mutex_key#14);
>   lock(&sess->cmdsn_mutex);
> 
>  *** DEADLOCK ***
> 
> 3 locks held by rmdir/13321:
>  #0:  (sb_writers#10){.+.+.+}, at: [<ffffffff811e1aff>] mnt_want_write+0x1f/0x50
>  #1:  (&default_group_class[depth - 1]#2/1){+.+.+.}, at: [<ffffffff811cc8ce>] do_rmdir+0x15e/0x200
>  #2:  (&sb->s_type->i_mutex_key#14){++++++}, at: [<ffffffff811c6e20>] vfs_rmdir+0x50/0x140
> 
> stack backtrace:
> CPU: 2 PID: 13321 Comm: rmdir Not tainted 4.10.0-rc7-dbg+ #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> Call Trace:
>  dump_stack+0x86/0xc3
>  print_circular_bug+0x1c7/0x220
>  __lock_acquire+0x10e6/0x1260
>  lock_acquire+0x71/0x90
>  mutex_lock_nested+0x5f/0x670
>  iscsit_free_all_ooo_cmdsns+0x2d/0xb0 [iscsi_target_mod]
>  iscsit_close_session+0xac/0x200 [iscsi_target_mod]
>  lio_tpg_close_session+0x9f/0xb0 [iscsi_target_mod]
>  target_shutdown_sessions+0xc3/0xd0 [target_core_mod]
>  core_tpg_del_initiator_node_acl+0x91/0x140 [target_core_mod]
>  target_fabric_nacl_base_release+0x20/0x30 [target_core_mod]
>  config_item_release+0x5a/0xc0 [configfs]
>  config_item_put+0x1d/0x1f [configfs]
>  configfs_rmdir+0x1a6/0x300 [configfs]
>  vfs_rmdir+0xb7/0x140
>  do_rmdir+0x1f4/0x200
>  SyS_rmdir+0x11/0x20
>  entry_SYSCALL_64_fastpath+0x23/0xc6
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_xcopy.c | 110 +++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 41 deletions(-)

Applied, but dropping the stable CC'.

In practice this deadlock has never triggered, so it's not stable
material.

[PATCH 09/33] configfs: Introduce config_item_get_unless_zero()

[Bart Van Assche]

This new function is needed to fix a deadlock in the SCSI target
XCOPY implementation.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/configfs/item.c       | 6 ++++++
 include/linux/configfs.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/fs/configfs/item.c b/fs/configfs/item.c
index 8b2a994042dd..e3501b9bbb60 100644
--- a/fs/configfs/item.c
+++ b/fs/configfs/item.c
@@ -138,6 +138,12 @@ struct config_item *config_item_get(struct config_item *item)
 }
 EXPORT_SYMBOL(config_item_get);
 
+struct config_item *config_item_get_unless_zero(struct config_item *item)
+{
+	return item && kref_get_unless_zero(&item->ci_kref) ? item : NULL;
+}
+EXPORT_SYMBOL(config_item_get_unless_zero);
+
 static void config_item_cleanup(struct config_item *item)
 {
 	struct config_item_type *t = item->ci_type;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2319b8c108e8..406e16dabc28 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -75,6 +75,7 @@ extern void config_item_init_type_name(struct config_item *item,
 				       struct config_item_type *type);
 
 extern struct config_item * config_item_get(struct config_item *);
+extern struct config_item * config_item_get_unless_zero(struct config_item *);
 extern void config_item_put(struct config_item *);
 
 struct config_item_type {
-- 
2.12.2

Re: [PATCH 09/33] configfs: Introduce config_item_get_unless_zero()

[Christoph Hellwig]

> +struct config_item *config_item_get_unless_zero(struct config_item *item)
> +{
> +	return item && kref_get_unless_zero(&item->ci_kref) ? item : NULL;
> +}
> +EXPORT_SYMBOL(config_item_get_unless_zero);

Style nipick, I'd prefer something like:

	if (item && !kref_get_unless_zero(&item->ci_kref))
		item = NULL;
	return item;

Otherwise this looks fine to me:

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

or should I pick it up through the configfs tree?

Re: [PATCH 09/33] configfs: Introduce config_item_get_unless_zero()

[Bart Van Assche]

On Sun, 2017-05-28 at 02:33 -0700, Christoph Hellwig wrote:
> > +struct config_item *config_item_get_unless_zero(struct config_item *item)
> > +{
> > +	return item && kref_get_unless_zero(&item->ci_kref) ? item : NULL;
> > +}
> > +EXPORT_SYMBOL(config_item_get_unless_zero);
> 
> Style nipick, I'd prefer something like:
> 
> 	if (item && !kref_get_unless_zero(&item->ci_kref))
> 		item = NULL;
> 	return item;
> 
> Otherwise this looks fine to me:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> or should I pick it up through the configfs tree?

Hello Christoph,

If you could pick up this patch (any style) through the configfs tree that would
be great.

Thanks,

Bart.

Re: [PATCH 09/33] configfs: Introduce config_item_get_unless_zero()

[Mike Christie]

On 05/23/2017 06:48 PM, Bart Van Assche wrote:
> This new function is needed to fix a deadlock in the SCSI target
> XCOPY implementation.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: linux-fsdevel@vger.kernel.org

Looks ok and test by me.

Reviewed-by: Mike Christie <mchristi@redhat.com>

[PATCH 15/33] xen/scsiback: Fix a use-after-free

[Bart Van Assche]

scsiback_release_cmd() must not dereference se_cmd->se_tmr_req
because that memory is freed by target_free_cmd_mem() before
scsiback_release_cmd() is called. Fix this use-after-free by
inlining struct scsiback_tmr into struct vscsibk_pend.

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

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index d6950e0802b7..980f32817305 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -134,9 +134,7 @@ struct vscsibk_pend {
 	struct page *pages[VSCSI_MAX_GRANTS];
 
 	struct se_cmd se_cmd;
-};
 
-struct scsiback_tmr {
 	atomic_t tmr_complete;
 	wait_queue_head_t tmr_wait;
 };
@@ -599,26 +597,20 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	struct scsiback_tpg *tpg = pending_req->v2p->tpg;
 	struct scsiback_nexus *nexus = tpg->tpg_nexus;
 	struct se_cmd *se_cmd = &pending_req->se_cmd;
-	struct scsiback_tmr *tmr;
 	u64 unpacked_lun = pending_req->v2p->lun;
 	int rc, err = FAILED;
 
-	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
-
-	init_waitqueue_head(&tmr->tmr_wait);
+	init_waitqueue_head(&pending_req->tmr_wait);
 
 	rc = target_submit_tmr(&pending_req->se_cmd, nexus->tvn_se_sess,
 			       &pending_req->sense_buffer[0],
-			       unpacked_lun, tmr, act, GFP_KERNEL,
+			       unpacked_lun, NULL, act, GFP_KERNEL,
 			       tag, TARGET_SCF_ACK_KREF);
 	if (rc)
 		goto err;
 
-	wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
+	wait_event(pending_req->tmr_wait,
+		   atomic_read(&pending_req->tmr_complete));
 
 	err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
 		SUCCESS : FAILED;
@@ -626,9 +618,8 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
 	transport_generic_free_cmd(&pending_req->se_cmd, 1);
 	return;
+
 err:
-	if (tmr)
-		kfree(tmr);
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
 }
 
@@ -1389,12 +1380,6 @@ static int scsiback_check_stop_free(struct se_cmd *se_cmd)
 static void scsiback_release_cmd(struct se_cmd *se_cmd)
 {
 	struct se_session *se_sess = se_cmd->se_sess;
-	struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
-
-	if (se_tmr && se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) {
-		struct scsiback_tmr *tmr = se_tmr->fabric_tmr_ptr;
-		kfree(tmr);
-	}
 
 	percpu_ida_free(&se_sess->sess_tag_pool, se_cmd->map_tag);
 }
@@ -1455,11 +1440,11 @@ static int scsiback_queue_status(struct se_cmd *se_cmd)
 
 static void scsiback_queue_tm_rsp(struct se_cmd *se_cmd)
 {
-	struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
-	struct scsiback_tmr *tmr = se_tmr->fabric_tmr_ptr;
+	struct vscsibk_pend *pending_req = container_of(se_cmd,
+				struct vscsibk_pend, se_cmd);
 
-	atomic_set(&tmr->tmr_complete, 1);
-	wake_up(&tmr->tmr_wait);
+	atomic_set(&pending_req->tmr_complete, 1);
+	wake_up(&pending_req->tmr_wait);
 }
 
 static void scsiback_aborted_task(struct se_cmd *se_cmd)
-- 
2.12.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 16/33] xen/scsiback: Replace a waitqueue and a counter by a completion

[Bart Van Assche]

This patch simplifies the implementation of the scsiback driver
but does not change its behavior.

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

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 980f32817305..4cb33a0916a8 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -135,8 +135,7 @@ struct vscsibk_pend {
 
 	struct se_cmd se_cmd;
 
-	atomic_t tmr_complete;
-	wait_queue_head_t tmr_wait;
+	struct completion tmr_done;
 };
 
 #define VSCSI_DEFAULT_SESSION_TAGS	128
@@ -600,7 +599,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	u64 unpacked_lun = pending_req->v2p->lun;
 	int rc, err = FAILED;
 
-	init_waitqueue_head(&pending_req->tmr_wait);
+	init_completion(&pending_req->tmr_done);
 
 	rc = target_submit_tmr(&pending_req->se_cmd, nexus->tvn_se_sess,
 			       &pending_req->sense_buffer[0],
@@ -609,8 +608,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	if (rc)
 		goto err;
 
-	wait_event(pending_req->tmr_wait,
-		   atomic_read(&pending_req->tmr_complete));
+	wait_for_completion(&pending_req->tmr_done);
 
 	err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
 		SUCCESS : FAILED;
@@ -1443,8 +1441,7 @@ static void scsiback_queue_tm_rsp(struct se_cmd *se_cmd)
 	struct vscsibk_pend *pending_req = container_of(se_cmd,
 				struct vscsibk_pend, se_cmd);
 
-	atomic_set(&pending_req->tmr_complete, 1);
-	wake_up(&pending_req->tmr_wait);
+	complete(&pending_req->tmr_done);
 }
 
 static void scsiback_aborted_task(struct se_cmd *se_cmd)
-- 
2.12.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 17/33] xen/scsiback: Make TMF processing slightly faster

[Bart Van Assche]

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

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

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 4cb33a0916a8..7bc88fd43cfc 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -614,7 +614,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 		SUCCESS : FAILED;
 
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
-	transport_generic_free_cmd(&pending_req->se_cmd, 1);
+	transport_generic_free_cmd(&pending_req->se_cmd, 0);
 	return;
 
 err:
-- 
2.12.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 25/33] target/iscsi: Avoid overflowing the receive buffer

[Bart Van Assche]

Since target_alloc_sgl() and iscsit_allocate_iovecs() allocate
buffer space for se_cmd.data_length bytes and since that number
can be smaller than the iSCSI Expected Data Transfer Length
(EDTL), ensure that the iSCSI target driver does not attempt to
receive more bytes than what fits in the receive buffer. Always
receive the full immediate data buffer.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: David Disseldorp <ddiss@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/target/iscsi/iscsi_target.c      | 27 ++++++++++++++++++++++++---
 drivers/target/iscsi/iscsi_target_util.c |  1 +
 include/target/iscsi/iscsi_target_core.h |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 86acf81c1cf9..c91604feca18 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1564,9 +1564,11 @@ iscsit_get_dataout(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 {
 	struct kvec *iov;
 	u32 checksum, iov_count = 0, padding = 0, rx_got = 0, rx_size = 0;
-	u32 payload_length = ntoh24(hdr->dlength);
+	u32 payload_length;
 	int iov_ret, data_crc_failed = 0;
 
+	payload_length = min_t(u32, cmd->se_cmd.data_length,
+			       ntoh24(hdr->dlength));
 	rx_size += payload_length;
 	iov = &cmd->iov_data[0];
 
@@ -2577,14 +2579,33 @@ static int iscsit_handle_immediate_data(
 	u32 checksum, iov_count = 0, padding = 0;
 	struct iscsi_conn *conn = cmd->conn;
 	struct kvec *iov;
+	void *overflow_buf = NULL;
 
-	iov_ret = iscsit_map_iovec(cmd, cmd->iov_data, cmd->write_data_done, length);
+	BUG_ON(cmd->write_data_done > cmd->se_cmd.data_length);
+	rx_size = min(cmd->se_cmd.data_length - cmd->write_data_done, length);
+	iov_ret = iscsit_map_iovec(cmd, cmd->iov_data, cmd->write_data_done,
+				   rx_size);
 	if (iov_ret < 0)
 		return IMMEDIATE_DATA_CANNOT_RECOVER;
 
-	rx_size = length;
 	iov_count = iov_ret;
 	iov = &cmd->iov_data[0];
+	if (rx_size < length) {
+		/*
+		 * Special case: length of immediate data exceeds the data
+		 * buffer size derived from the CDB (overflow).
+		 */
+		overflow_buf = kmalloc(length - rx_size, GFP_KERNEL);
+		if (!overflow_buf) {
+			iscsit_unmap_iovec(cmd);
+			return IMMEDIATE_DATA_CANNOT_RECOVER;
+		}
+		cmd->overflow_buf = overflow_buf;
+		iov[iov_count].iov_base = overflow_buf;
+		iov[iov_count].iov_len = length - rx_size;
+		iov_count++;
+		rx_size = length;
+	}
 
 	padding = ((-length) & 3);
 	if (padding != 0) {
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 1e36f83b5961..68371a7e54d8 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -705,6 +705,7 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd)
 	kfree(cmd->pdu_list);
 	kfree(cmd->seq_list);
 	kfree(cmd->tmr_req);
+	kfree(cmd->overflow_buf);
 	kfree(cmd->iov_data);
 	kfree(cmd->text_in_ptr);
 
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 275581d483dd..968825200ecb 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -462,6 +462,7 @@ struct iscsi_cmd {
 	struct timer_list	dataout_timer;
 	/* Iovecs for SCSI data payload RX/TX w/ kernel level sockets */
 	struct kvec		*iov_data;
+	void			*overflow_buf;
 	/* Iovecs for miscellaneous purposes */
 #define ISCSI_MISC_IOVECS			5
 	struct kvec		iov_misc[ISCSI_MISC_IOVECS];
-- 
2.12.2

[PATCH 29/33] target/iscsi: Simplify timer manipulation code

[Bart Van Assche]

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

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

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

Re: [PATCH 15/33] xen/scsiback: Fix a use-after-free

[Juergen Gross]

On 24/05/17 01:48, Bart Van Assche wrote:
> scsiback_release_cmd() must not dereference se_cmd->se_tmr_req
> because that memory is freed by target_free_cmd_mem() before
> scsiback_release_cmd() is called. Fix this use-after-free by
> inlining struct scsiback_tmr into struct vscsibk_pend.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 15/33] xen/scsiback: Fix a use-after-free

[Nicholas A. Bellinger]

On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> scsiback_release_cmd() must not dereference se_cmd->se_tmr_req
> because that memory is freed by target_free_cmd_mem() before
> scsiback_release_cmd() is called. Fix this use-after-free by
> inlining struct scsiback_tmr into struct vscsibk_pend.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org
> ---
>  drivers/xen/xen-scsiback.c | 33 +++++++++------------------------
>  1 file changed, 9 insertions(+), 24 deletions(-)

Applied.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 15/33] xen/scsiback: Fix a use-after-free

[Nicholas A. Bellinger]

On Fri, 2017-06-02 at 22:40 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > scsiback_release_cmd() must not dereference se_cmd->se_tmr_req
> > because that memory is freed by target_free_cmd_mem() before
> > scsiback_release_cmd() is called. Fix this use-after-free by
> > inlining struct scsiback_tmr into struct vscsibk_pend.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: David Disseldorp <ddiss@suse.de>
> > Cc: xen-devel@lists.xenproject.org
> > ---
> >  drivers/xen/xen-scsiback.c | 33 +++++++++------------------------
> >  1 file changed, 9 insertions(+), 24 deletions(-)
> 
> Applied.
> 

Oh btw, this looks like stable material to me.

So unless Juergen has any objections, adding a v3.18+ tag.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 15/33] xen/scsiback: Fix a use-after-free

[Juergen Gross]

On 03/06/17 09:04, Nicholas A. Bellinger wrote:
> On Fri, 2017-06-02 at 22:40 -0700, Nicholas A. Bellinger wrote:
>> On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
>>> scsiback_release_cmd() must not dereference se_cmd->se_tmr_req
>>> because that memory is freed by target_free_cmd_mem() before
>>> scsiback_release_cmd() is called. Fix this use-after-free by
>>> inlining struct scsiback_tmr into struct vscsibk_pend.
>>>
>>> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Hannes Reinecke <hare@suse.com>
>>> Cc: David Disseldorp <ddiss@suse.de>
>>> Cc: xen-devel@lists.xenproject.org
>>> ---
>>>  drivers/xen/xen-scsiback.c | 33 +++++++++------------------------
>>>  1 file changed, 9 insertions(+), 24 deletions(-)
>>
>> Applied.
>>
> 
> Oh btw, this looks like stable material to me.
> 
> So unless Juergen has any objections, adding a v3.18+ tag.

No objections from me.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 16/33] xen/scsiback: Replace a waitqueue and a counter by a completion

[Juergen Gross]

On 24/05/17 01:48, Bart Van Assche wrote:
> This patch simplifies the implementation of the scsiback driver
> but does not change its behavior.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 16/33] xen/scsiback: Replace a waitqueue and a counter by a completion

[Nicholas A. Bellinger]

On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> This patch simplifies the implementation of the scsiback driver
> but does not change its behavior.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org
> ---
>  drivers/xen/xen-scsiback.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Applied.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 17/33] xen/scsiback: Make TMF processing slightly faster

[Juergen Gross]

On 24/05/17 01:48, Bart Van Assche wrote:
> Target drivers must guarantee that struct se_cmd and struct se_tmr_req
> exist as long as target_tmr_work() is in progress. Since the last
> access by the LIO core is a call to .check_stop_free() and since the
> Xen scsiback .check_stop_free() drops a reference to the TMF, it is
> already guaranteed that the struct se_cmd that corresponds to the TMF
> exists as long as target_tmr_work() is in progress. Hence change the
> second argument of transport_generic_free_cmd() from 1 into 0.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 17/33] xen/scsiback: Make TMF processing slightly faster

[Nicholas A. Bellinger]

On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> Target drivers must guarantee that struct se_cmd and struct se_tmr_req
> exist as long as target_tmr_work() is in progress. Since the last
> access by the LIO core is a call to .check_stop_free() and since the
> Xen scsiback .check_stop_free() drops a reference to the TMF, it is
> already guaranteed that the struct se_cmd that corresponds to the TMF
> exists as long as target_tmr_work() is in progress. Hence change the
> second argument of transport_generic_free_cmd() from 1 into 0.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: xen-devel@lists.xenproject.org
> ---
>  drivers/xen/xen-scsiback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel