[PATCH 00/11] lots of fixes for zero allocation length

[PATCH 00/11] lots of fixes for zero allocation length

[Paolo Bonzini]

Here are the fixes for zero-length commands, that ultimately let
all command go through normal processing, even if they have
zero length.

The first patch is for PSCSI, the others are for IBLOCK and friends.
I tried to order them so that the safest come first.  So for 3.6 you
could choose to stop at patch 1, 2, 3 or 6, or not apply anything
at all.

Testcases included in each patch, when relevant.

Paolo

Paolo Bonzini (11):
  target: go through normal processing for zero-length PSCSI commands
  target: report too-small parameter lists everywhere
  target: fail REPORT LUNS with less than 16 bytes of payload
  target: support zero-size allocation lengths in transport_kmap_data_sg
  target: support zero allocation length in REQUEST SENSE
  target: go through normal processing for zero-length REQUEST_SENSE
  target: support zero allocation length in INQUIRY
  target: fix truncation of mode data, support zero allocation length
  target: support zero allocation length in SBC commands
  target: do not submit a zero-bio I/O request
  target: go through normal processing for all zero-length commands

 drivers/target/target_core_alua.c      |    7 +++
 drivers/target/target_core_device.c    |    7 +++
 drivers/target/target_core_iblock.c    |   17 ++++++-
 drivers/target/target_core_pr.c        |    8 +++
 drivers/target/target_core_pscsi.c     |    8 ++--
 drivers/target/target_core_sbc.c       |   23 ++++++---
 drivers/target/target_core_spc.c       |   81 +++++++++++--------------------
 drivers/target/target_core_transport.c |   35 ++++----------
 include/target/target_core_base.h      |    1 +
 9 files changed, 95 insertions(+), 92 deletions(-)

[PATCH 01/11] target: go through normal processing for zero-length PSCSI commands

[Paolo Bonzini]

Right now, commands with a zero-size payload are skipped completely.
This is wrong; such commands should be passed down to the device and
processed normally.

For physical backends, this ignores completely things such as START
STOP UNIT.  For virtual backends, we have a hack in place to clear a
unit attention state on a zero-size REQUEST SENSE, but we still do
not report errors properly on zero-length commands---out-of-bounds
0-block reads and writes, too small parameter list lengths, etc.

This patch fixes this for PSCSI.  Uses of transport_kmap_data_sg are
guarded with a check for non-zero cmd->data_length; for all other
commands a zero length is handled properly in pscsi_execute_cmd.
The sole exception will be for now REPORT LUNS, which is handled
through the normal SPC emulation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_pscsi.c     |    8 ++++----
 drivers/target/target_core_transport.c |    4 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index c026ee3..682a581 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -688,11 +688,11 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
 	 * Hack to make sure that Write-Protect modepage is set if R/O mode is
 	 * forced.
 	 */
+	if (!cmd->se_deve || !cmd->data_length)
+		goto after_mode_sense;
+
 	if (((cdb[0] == MODE_SENSE) || (cdb[0] == MODE_SENSE_10)) &&
 	     (status_byte(result) << 1) == SAM_STAT_GOOD) {
-		if (!cmd->se_deve)
-			goto after_mode_sense;
-
 		if (cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY) {
 			unsigned char *buf = transport_kmap_data_sg(cmd);
 
@@ -709,7 +709,7 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
 	}
 after_mode_sense:
 
-	if (sd->type != TYPE_TAPE)
+	if (sd->type != TYPE_TAPE || !cmd->data_length)
 		goto after_mode_select;
 
 	/*
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 09028af..2e55aa9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2295,7 +2295,9 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
 	 * into the fabric for data transfers, go ahead and complete it right
 	 * away.
 	 */
-	if (!cmd->data_length) {
+	if (!cmd->data_length &&
+	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
+	     cmd->t_task_cdb[0] == REPORT_LUNS) {
 		spin_lock_irq(&cmd->t_state_lock);
 		cmd->t_state = TRANSPORT_COMPLETE;
 		cmd->transport_state |= CMD_T_ACTIVE;
-- 
1.7.1

Re: [PATCH 01/11] target: go through normal processing for zero-length PSCSI commands

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> Right now, commands with a zero-size payload are skipped completely.
> This is wrong; such commands should be passed down to the device and
> processed normally.
> 
> For physical backends, this ignores completely things such as START
> STOP UNIT.  For virtual backends, we have a hack in place to clear a
> unit attention state on a zero-size REQUEST SENSE, but we still do
> not report errors properly on zero-length commands---out-of-bounds
> 0-block reads and writes, too small parameter list lengths, etc.
> 
> This patch fixes this for PSCSI.  Uses of transport_kmap_data_sg are
> guarded with a check for non-zero cmd->data_length; for all other
> commands a zero length is handled properly in pscsi_execute_cmd.
> The sole exception will be for now REPORT LUNS, which is handled
> through the normal SPC emulation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to target-pending/master for-3.6.

Thanks Paolo!

> ---
>  drivers/target/target_core_pscsi.c     |    8 ++++----
>  drivers/target/target_core_transport.c |    4 +++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index c026ee3..682a581 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -688,11 +688,11 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
>  	 * Hack to make sure that Write-Protect modepage is set if R/O mode is
>  	 * forced.
>  	 */
> +	if (!cmd->se_deve || !cmd->data_length)
> +		goto after_mode_sense;
> +
>  	if (((cdb[0] == MODE_SENSE) || (cdb[0] == MODE_SENSE_10)) &&
>  	     (status_byte(result) << 1) == SAM_STAT_GOOD) {
> -		if (!cmd->se_deve)
> -			goto after_mode_sense;
> -
>  		if (cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY) {
>  			unsigned char *buf = transport_kmap_data_sg(cmd);
>  
> @@ -709,7 +709,7 @@ static void pscsi_transport_complete(struct se_cmd *cmd, struct scatterlist *sg,
>  	}
>  after_mode_sense:
>  
> -	if (sd->type != TYPE_TAPE)
> +	if (sd->type != TYPE_TAPE || !cmd->data_length)
>  		goto after_mode_select;
>  
>  	/*
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 09028af..2e55aa9 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2295,7 +2295,9 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>  	 * into the fabric for data transfers, go ahead and complete it right
>  	 * away.
>  	 */
> -	if (!cmd->data_length) {
> +	if (!cmd->data_length &&
> +	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
> +	     cmd->t_task_cdb[0] == REPORT_LUNS) {
>  		spin_lock_irq(&cmd->t_state_lock);
>  		cmd->t_state = TRANSPORT_COMPLETE;
>  		cmd->transport_state |= CMD_T_ACTIVE;

[PATCH 02/11] target: report too-small parameter lists everywhere

[Paolo Bonzini]

Several places were not checking that the parameter list length
was large enough, and thus accessing invalid memory.  Zero-length
parameter lists are just a special case of this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_alua.c   |    7 +++++++
 drivers/target/target_core_iblock.c |   17 +++++++++++++++--
 drivers/target/target_core_pr.c     |    8 ++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 9179997..41641ba 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -218,6 +218,13 @@ int target_emulate_set_target_port_groups(struct se_cmd *cmd)
 		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		return -EINVAL;
 	}
+	if (cmd->data_length < 4) {
+		pr_warn("SET TARGET PORT GROUPS parameter list length %u too"
+			" small\n", cmd->data_length);
+		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
+		return -EINVAL;
+	}
+
 	buf = transport_kmap_data_sg(cmd);
 
 	/*
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 76db75e..9ba4954 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -325,17 +325,30 @@ static int iblock_execute_unmap(struct se_cmd *cmd)
 	struct iblock_dev *ibd = dev->dev_ptr;
 	unsigned char *buf, *ptr = NULL;
 	sector_t lba;
-	int size = cmd->data_length;
+	int size;
 	u32 range;
 	int ret = 0;
 	int dl, bd_dl;
 
+	if (cmd->data_length < 8) {
+		pr_warn("UNMAP parameter list length %u too small\n",
+			cmd->data_length);
+		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
+		return -EINVAL;
+	}
+
 	buf = transport_kmap_data_sg(cmd);
 
 	dl = get_unaligned_be16(&buf[0]);
 	bd_dl = get_unaligned_be16(&buf[2]);
 
-	size = min(size - 8, bd_dl);
+	size = cmd->data_length - 8;
+	if (bd_dl > size)
+		pr_warn("UNMAP parameter list length %u too small, ignoring bd_dl %u\n",
+			cmd->data_length, bd_dl);
+	else
+		size = bd_dl;
+
 	if (size / 16 > dev->se_sub_dev->se_dev_attrib.max_unmap_block_desc_count) {
 		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
 		ret = -EINVAL;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 1e94650..956c84c 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1540,6 +1540,14 @@ static int core_scsi3_decode_spec_i_port(
 	tidh_new->dest_local_nexus = 1;
 	list_add_tail(&tidh_new->dest_list, &tid_dest_list);
 
+	if (cmd->data_length < 28) {
+		pr_warn("SPC-PR: Received PR OUT parameter list"
+			" length too small: %u\n", cmd->data_length);
+		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
+		ret = -EINVAL;
+		goto out;
+	}
+
 	buf = transport_kmap_data_sg(cmd);
 	/*
 	 * For a PERSISTENT RESERVE OUT specify initiator ports payload,
-- 
1.7.1

Re: [PATCH 02/11] target: report too-small parameter lists everywhere

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> Several places were not checking that the parameter list length
> was large enough, and thus accessing invalid memory.  Zero-length
> parameter lists are just a special case of this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

So I think this looks like reasonable for-3.6 material as well.

Roland, do you have any objections to this..?

>  drivers/target/target_core_alua.c   |    7 +++++++
>  drivers/target/target_core_iblock.c |   17 +++++++++++++++--
>  drivers/target/target_core_pr.c     |    8 ++++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> index 9179997..41641ba 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -218,6 +218,13 @@ int target_emulate_set_target_port_groups(struct se_cmd *cmd)
>  		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		return -EINVAL;
>  	}
> +	if (cmd->data_length < 4) {
> +		pr_warn("SET TARGET PORT GROUPS parameter list length %u too"
> +			" small\n", cmd->data_length);
> +		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> +		return -EINVAL;
> +	}
> +
>  	buf = transport_kmap_data_sg(cmd);
>  
>  	/*
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index 76db75e..9ba4954 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -325,17 +325,30 @@ static int iblock_execute_unmap(struct se_cmd *cmd)
>  	struct iblock_dev *ibd = dev->dev_ptr;
>  	unsigned char *buf, *ptr = NULL;
>  	sector_t lba;
> -	int size = cmd->data_length;
> +	int size;
>  	u32 range;
>  	int ret = 0;
>  	int dl, bd_dl;
>  
> +	if (cmd->data_length < 8) {
> +		pr_warn("UNMAP parameter list length %u too small\n",
> +			cmd->data_length);
> +		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> +		return -EINVAL;
> +	}
> +
>  	buf = transport_kmap_data_sg(cmd);
>  
>  	dl = get_unaligned_be16(&buf[0]);
>  	bd_dl = get_unaligned_be16(&buf[2]);
>  
> -	size = min(size - 8, bd_dl);
> +	size = cmd->data_length - 8;
> +	if (bd_dl > size)
> +		pr_warn("UNMAP parameter list length %u too small, ignoring bd_dl %u\n",
> +			cmd->data_length, bd_dl);
> +	else
> +		size = bd_dl;
> +
>  	if (size / 16 > dev->se_sub_dev->se_dev_attrib.max_unmap_block_desc_count) {
>  		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
>  		ret = -EINVAL;
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 1e94650..956c84c 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1540,6 +1540,14 @@ static int core_scsi3_decode_spec_i_port(
>  	tidh_new->dest_local_nexus = 1;
>  	list_add_tail(&tidh_new->dest_list, &tid_dest_list);
>  
> +	if (cmd->data_length < 28) {
> +		pr_warn("SPC-PR: Received PR OUT parameter list"
> +			" length too small: %u\n", cmd->data_length);
> +		cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	buf = transport_kmap_data_sg(cmd);
>  	/*
>  	 * For a PERSISTENT RESERVE OUT specify initiator ports payload,

[PATCH 03/11] target: fail REPORT LUNS with less than 16 bytes of payload

[Paolo Bonzini]

SPC says:

"The ALLOCATION LENGTH field is defined in 4.3.5.6. The allocation length
should be at least 16.  Device servers compliant with SPC return CHECK
CONDITION status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB when the allocation
length is less than 16 bytes".

Testcase: sg_raw -r8 /dev/sdb a0 00 00 00 00 00 00 00 00 08 00 00
    should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense
    does not fail without the patch
    fails correctly with the patch

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_device.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index cf2c66f..9fc9a60 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -669,6 +669,13 @@ int target_report_luns(struct se_cmd *se_cmd)
 	unsigned char *buf;
 	u32 lun_count = 0, offset = 8, i;
 
+	if (se_cmd->data_length < 16) {
+		pr_warn("REPORT LUNS allocation length %u too small\n",
+			se_cmd->data_length);
+		se_cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
+		return -EINVAL;
+	}
+
 	buf = transport_kmap_data_sg(se_cmd);
 	if (!buf)
 		return -ENOMEM;
-- 
1.7.1

Re: [PATCH 03/11] target: fail REPORT LUNS with less than 16 bytes of payload

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> SPC says:
> 
> "The ALLOCATION LENGTH field is defined in 4.3.5.6. The allocation length
> should be at least 16.  Device servers compliant with SPC return CHECK
> CONDITION status, with the sense key set to ILLEGAL REQUEST, and the
> additional sense code set to INVALID FIELD IN CDB when the allocation
> length is less than 16 bytes".
> 
> Testcase: sg_raw -r8 /dev/sdb a0 00 00 00 00 00 00 00 00 08 00 00
>     should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense
>     does not fail without the patch
>     fails correctly with the patch
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Looks good.  Applied to master.

Thanks Paolo!

>  drivers/target/target_core_device.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index cf2c66f..9fc9a60 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -669,6 +669,13 @@ int target_report_luns(struct se_cmd *se_cmd)
>  	unsigned char *buf;
>  	u32 lun_count = 0, offset = 8, i;
>  
> +	if (se_cmd->data_length < 16) {
> +		pr_warn("REPORT LUNS allocation length %u too small\n",
> +			se_cmd->data_length);
> +		se_cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
> +		return -EINVAL;
> +	}
> +
>  	buf = transport_kmap_data_sg(se_cmd);
>  	if (!buf)
>  		return -ENOMEM;

[PATCH 04/11] target: support zero-size allocation lengths in transport_kmap_data_sg

[Paolo Bonzini]

In order to support zero-size allocation lengths, do not assert
that we have a scatterlist until after checking cmd->data_length.

But once we do this, we can have two cases of transport_kmap_data_sg
returning NULL: a zero-size allocation length, or an out-of-memory
condition.  Report the latter using sense codes, so that the SCSI
command that triggered it will fail.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_transport.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2e55aa9..8facb74 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2185,7 +2185,6 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
 	struct page **pages;
 	int i;
 
-	BUG_ON(!sg);
 	/*
 	 * We need to take into account a possible offset here for fabrics like
 	 * tcm_loop who may be using a contig buffer from the SCSI midlayer for
@@ -2193,13 +2192,17 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
 	 */
 	if (!cmd->t_data_nents)
 		return NULL;
-	else if (cmd->t_data_nents == 1)
+
+	BUG_ON(!sg);
+	if (cmd->t_data_nents == 1)
 		return kmap(sg_page(sg)) + sg->offset;
 
 	/* >1 page. use vmap */
 	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
-	if (!pages)
+	if (!pages) {
+		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		return NULL;
+	}
 
 	/* convert sg[] to pages[] */
 	for_each_sg(cmd->t_data_sg, sg, cmd->t_data_nents, i) {
@@ -2208,8 +2211,10 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
 
 	cmd->t_data_vmap = vmap(pages, cmd->t_data_nents,  VM_MAP, PAGE_KERNEL);
 	kfree(pages);
-	if (!cmd->t_data_vmap)
+	if (!cmd->t_data_vmap) {
+		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		return NULL;
+	}
 
 	return cmd->t_data_vmap + cmd->t_data_sg[0].offset;
 }
-- 
1.7.1

Re: [PATCH 04/11] target: support zero-size allocation lengths in transport_kmap_data_sg

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> In order to support zero-size allocation lengths, do not assert
> that we have a scatterlist until after checking cmd->data_length.
> 
> But once we do this, we can have two cases of transport_kmap_data_sg
> returning NULL: a zero-size allocation length, or an out-of-memory
> condition.  Report the latter using sense codes, so that the SCSI
> command that triggered it will fail.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Applied to master for-3.6.

Nice work Paolo!

>  drivers/target/target_core_transport.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 2e55aa9..8facb74 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2185,7 +2185,6 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
>  	struct page **pages;
>  	int i;
>  
> -	BUG_ON(!sg);
>  	/*
>  	 * We need to take into account a possible offset here for fabrics like
>  	 * tcm_loop who may be using a contig buffer from the SCSI midlayer for
> @@ -2193,13 +2192,17 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
>  	 */
>  	if (!cmd->t_data_nents)
>  		return NULL;
> -	else if (cmd->t_data_nents == 1)
> +
> +	BUG_ON(!sg);
> +	if (cmd->t_data_nents == 1)
>  		return kmap(sg_page(sg)) + sg->offset;
>  
>  	/* >1 page. use vmap */
>  	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
> -	if (!pages)
> +	if (!pages) {
> +		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		return NULL;
> +	}
>  
>  	/* convert sg[] to pages[] */
>  	for_each_sg(cmd->t_data_sg, sg, cmd->t_data_nents, i) {
> @@ -2208,8 +2211,10 @@ void *transport_kmap_data_sg(struct se_cmd *cmd)
>  
>  	cmd->t_data_vmap = vmap(pages, cmd->t_data_nents,  VM_MAP, PAGE_KERNEL);
>  	kfree(pages);
> -	if (!cmd->t_data_vmap)
> +	if (!cmd->t_data_vmap) {
> +		cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		return NULL;
> +	}
>  
>  	return cmd->t_data_vmap + cmd->t_data_sg[0].offset;
>  }

[PATCH 05/11] target: support zero allocation length in REQUEST SENSE

[Paolo Bonzini]

Similar to INQUIRY and MODE SENSE, construct the sense data in a
buffer and later copy it to the scatterlist.  Do not do anything,
but still clear a pending unit attention condition, if the allocation
length is zero.

However, SPC tells us that "If a REQUEST SENSE command is terminated with
CHECK CONDITION status [and] the REQUEST SENSE command was received on
an I_T nexus with a pending unit attention condition (i.e., before the
device server reports CHECK CONDITION status), then the device server
shall not clear the pending unit attention condition."  Do the
transport_kmap_data_sg early to detect this case.

It also tells us "Device servers shall not adjust the additional sense
length to reflect truncation if the allocation length is less than the
sense data available", so do not do that!  Note that the err variable
is write-only.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_spc.c  |   35 ++++++++++++++++++-----------------
 include/target/target_core_base.h |    1 +
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 4c861de..b905fb2 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -877,9 +877,11 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
 static int spc_emulate_request_sense(struct se_cmd *cmd)
 {
 	unsigned char *cdb = cmd->t_task_cdb;
-	unsigned char *buf;
+	unsigned char *rbuf;
 	u8 ua_asc = 0, ua_ascq = 0;
-	int err = 0;
+	unsigned char buf[SE_SENSE_BUF];
+
+	memset(buf, 0, SE_SENSE_BUF);
 
 	if (cdb[1] & 0x01) {
 		pr_err("REQUEST_SENSE description emulation not"
@@ -888,20 +890,21 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
 		return -ENOSYS;
 	}
 
-	buf = transport_kmap_data_sg(cmd);
-
-	if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) {
+	rbuf = transport_kmap_data_sg(cmd);
+	if (cmd->scsi_sense_reason != 0) {
+		/*
+		 * Out of memory.  We will fail with CHECK CONDITION, so
+		 * we must not clear the unit attention condition.
+		 */
+		target_complete_cmd(cmd, CHECK_CONDITION);
+		return 0;
+	} else if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) {
 		/*
 		 * CURRENT ERROR, UNIT ATTENTION
 		 */
 		buf[0] = 0x70;
 		buf[SPC_SENSE_KEY_OFFSET] = UNIT_ATTENTION;
 
-		if (cmd->data_length < 18) {
-			buf[7] = 0x00;
-			err = -EINVAL;
-			goto end;
-		}
 		/*
 		 * The Additional Sense Code (ASC) from the UNIT ATTENTION
 		 */
@@ -915,11 +916,6 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
 		buf[0] = 0x70;
 		buf[SPC_SENSE_KEY_OFFSET] = NO_SENSE;
 
-		if (cmd->data_length < 18) {
-			buf[7] = 0x00;
-			err = -EINVAL;
-			goto end;
-		}
 		/*
 		 * NO ADDITIONAL SENSE INFORMATION
 		 */
@@ -927,8 +923,11 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
 		buf[7] = 0x0A;
 	}
 
-end:
-	transport_kunmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
+		transport_kunmap_data_sg(cmd);
+	}
+
 	target_complete_cmd(cmd, GOOD);
 	return 0;
 }
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 015cea0..5be8937 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -121,6 +121,7 @@
 
 #define SE_INQUIRY_BUF				512
 #define SE_MODE_PAGE_BUF			512
+#define SE_SENSE_BUF				96
 
 /* struct se_hba->hba_flags */
 enum hba_flags_table {
-- 
1.7.1

Re: [PATCH 05/11] target: support zero allocation length in REQUEST SENSE

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> Similar to INQUIRY and MODE SENSE, construct the sense data in a
> buffer and later copy it to the scatterlist.  Do not do anything,
> but still clear a pending unit attention condition, if the allocation
> length is zero.
> 
> However, SPC tells us that "If a REQUEST SENSE command is terminated with
> CHECK CONDITION status [and] the REQUEST SENSE command was received on
> an I_T nexus with a pending unit attention condition (i.e., before the
> device server reports CHECK CONDITION status), then the device server
> shall not clear the pending unit attention condition."  Do the
> transport_kmap_data_sg early to detect this case.
> 
> It also tells us "Device servers shall not adjust the additional sense
> length to reflect truncation if the allocation length is less than the
> sense data available", so do not do that!  Note that the err variable
> is write-only.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Ditto on this one as well.  Applied to target-pending/master.

Thanks Paolo!

>  drivers/target/target_core_spc.c  |   35 ++++++++++++++++++-----------------
>  include/target/target_core_base.h |    1 +
>  2 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 4c861de..b905fb2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -877,9 +877,11 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
>  static int spc_emulate_request_sense(struct se_cmd *cmd)
>  {
>  	unsigned char *cdb = cmd->t_task_cdb;
> -	unsigned char *buf;
> +	unsigned char *rbuf;
>  	u8 ua_asc = 0, ua_ascq = 0;
> -	int err = 0;
> +	unsigned char buf[SE_SENSE_BUF];
> +
> +	memset(buf, 0, SE_SENSE_BUF);
>  
>  	if (cdb[1] & 0x01) {
>  		pr_err("REQUEST_SENSE description emulation not"
> @@ -888,20 +890,21 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
>  		return -ENOSYS;
>  	}
>  
> -	buf = transport_kmap_data_sg(cmd);
> -
> -	if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) {
> +	rbuf = transport_kmap_data_sg(cmd);
> +	if (cmd->scsi_sense_reason != 0) {
> +		/*
> +		 * Out of memory.  We will fail with CHECK CONDITION, so
> +		 * we must not clear the unit attention condition.
> +		 */
> +		target_complete_cmd(cmd, CHECK_CONDITION);
> +		return 0;
> +	} else if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) {
>  		/*
>  		 * CURRENT ERROR, UNIT ATTENTION
>  		 */
>  		buf[0] = 0x70;
>  		buf[SPC_SENSE_KEY_OFFSET] = UNIT_ATTENTION;
>  
> -		if (cmd->data_length < 18) {
> -			buf[7] = 0x00;
> -			err = -EINVAL;
> -			goto end;
> -		}
>  		/*
>  		 * The Additional Sense Code (ASC) from the UNIT ATTENTION
>  		 */
> @@ -915,11 +916,6 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
>  		buf[0] = 0x70;
>  		buf[SPC_SENSE_KEY_OFFSET] = NO_SENSE;
>  
> -		if (cmd->data_length < 18) {
> -			buf[7] = 0x00;
> -			err = -EINVAL;
> -			goto end;
> -		}
>  		/*
>  		 * NO ADDITIONAL SENSE INFORMATION
>  		 */
> @@ -927,8 +923,11 @@ static int spc_emulate_request_sense(struct se_cmd *cmd)
>  		buf[7] = 0x0A;
>  	}
>  
> -end:
> -	transport_kunmap_data_sg(cmd);
> +	if (rbuf) {
> +		memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
> +		transport_kunmap_data_sg(cmd);
> +	}
> +
>  	target_complete_cmd(cmd, GOOD);
>  	return 0;
>  }
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 015cea0..5be8937 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -121,6 +121,7 @@
>  
>  #define SE_INQUIRY_BUF				512
>  #define SE_MODE_PAGE_BUF			512
> +#define SE_SENSE_BUF				96
>  
>  /* struct se_hba->hba_flags */
>  enum hba_flags_table {

[PATCH 06/11] target: go through normal processing for zero-length REQUEST_SENSE

[Paolo Bonzini]

Now that spc_emulate_request_sense has been taught to process zero-length
REQUEST SENSE correctly, drop the special handling of unit attention
conditions from transport_generic_new_cmd.  However, for now REQUEST SENSE
will be the only command that goes through emulation for zero lengths.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_transport.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8facb74..09d9279 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2301,6 +2301,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
 	 * away.
 	 */
 	if (!cmd->data_length &&
+	    cmd->t_task_cdb[0] != REQUEST_SENSE &&
 	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
 	     cmd->t_task_cdb[0] == REPORT_LUNS) {
 		spin_lock_irq(&cmd->t_state_lock);
@@ -2308,13 +2309,6 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
 		cmd->transport_state |= CMD_T_ACTIVE;
 		spin_unlock_irq(&cmd->t_state_lock);
 
-		if (cmd->t_task_cdb[0] == REQUEST_SENSE) {
-			u8 ua_asc = 0, ua_ascq = 0;
-
-			core_scsi3_ua_clear_for_request_sense(cmd,
-					&ua_asc, &ua_ascq);
-		}
-
 		INIT_WORK(&cmd->work, target_complete_ok_work);
 		queue_work(target_completion_wq, &cmd->work);
 		return 0;
-- 
1.7.1

Re: [PATCH 06/11] target: go through normal processing for zero-length REQUEST_SENSE

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> Now that spc_emulate_request_sense has been taught to process zero-length
> REQUEST SENSE correctly, drop the special handling of unit attention
> conditions from transport_generic_new_cmd.  However, for now REQUEST SENSE
> will be the only command that goes through emulation for zero lengths.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

and also applied to master for-3.6.  Nice work Paolo!

So given the nature of these changes I'll likely wait end up waiting
until -rc6 to push these pending for-3.6 bugfixes.

Roland & hch, please let us know if you have any concerns.

Thank you!

--nab

>  drivers/target/target_core_transport.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 8facb74..09d9279 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2301,6 +2301,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>  	 * away.
>  	 */
>  	if (!cmd->data_length &&
> +	    cmd->t_task_cdb[0] != REQUEST_SENSE &&
>  	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
>  	     cmd->t_task_cdb[0] == REPORT_LUNS) {
>  		spin_lock_irq(&cmd->t_state_lock);
> @@ -2308,13 +2309,6 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>  		cmd->transport_state |= CMD_T_ACTIVE;
>  		spin_unlock_irq(&cmd->t_state_lock);
>  
> -		if (cmd->t_task_cdb[0] == REQUEST_SENSE) {
> -			u8 ua_asc = 0, ua_ascq = 0;
> -
> -			core_scsi3_ua_clear_for_request_sense(cmd,
> -					&ua_asc, &ua_ascq);
> -		}
> -
>  		INIT_WORK(&cmd->work, target_complete_ok_work);
>  		queue_work(target_completion_wq, &cmd->work);
>  		return 0;

Re: [PATCH 06/11] target: go through normal processing for zero-length REQUEST_SENSE

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 11:23 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> > Now that spc_emulate_request_sense has been taught to process zero-length
> > REQUEST SENSE correctly, drop the special handling of unit attention
> > conditions from transport_generic_new_cmd.  However, for now REQUEST SENSE
> > will be the only command that goes through emulation for zero lengths.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---

<SNIP>

> >  drivers/target/target_core_transport.c |    8 +-------
> >  1 files changed, 1 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 8facb74..09d9279 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2301,6 +2301,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
> >  	 * away.
> >  	 */
> >  	if (!cmd->data_length &&
> > +	    cmd->t_task_cdb[0] != REQUEST_SENSE &&
> >  	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
> >  	     cmd->t_task_cdb[0] == REPORT_LUNS) {
> >  		spin_lock_irq(&cmd->t_state_lock);

Btw, this section does not compile.

I'm squashing the following into your original patch.  Let me know if
you have any objections.

Thanks Paolo!

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index cda32eb..269f544 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2307,8 +2307,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
         */
        if (!cmd->data_length &&
            cmd->t_task_cdb[0] != REQUEST_SENSE &&
-           (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
-            cmd->t_task_cdb[0] == REPORT_LUNS) {
+           cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV) {
                spin_lock_irq(&cmd->t_state_lock);
                cmd->t_state = TRANSPORT_COMPLETE;
                cmd->transport_state |= CMD_T_ACTIVE;

Re: [PATCH 06/11] target: go through normal processing for zero-length REQUEST_SENSE

[Paolo Bonzini]

Il 07/09/2012 20:33, Nicholas A. Bellinger ha scritto:
>>> > >  	if (!cmd->data_length &&
>>> > > +	    cmd->t_task_cdb[0] != REQUEST_SENSE &&
>>> > >  	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
>>> > >  	     cmd->t_task_cdb[0] == REPORT_LUNS) {
>>> > >  		spin_lock_irq(&cmd->t_state_lock);
> Btw, this section does not compile.

The idea was to add the missing parenthesis to

cmd->t_task_cdb[0] == REPORT_LUNS))

but your fix is also correct after patch 3.

Thanks!

Paolo

[PATCH 07/11] target: support zero allocation length in INQUIRY

[Paolo Bonzini]

INQUIRY processing already uses an on-heap bounce buffer for loopback,
but not for other fabrics.  Switch this to a cheaper on-stack bounce
buffer, similar to the one used by MODE SENSE and REQUEST SENSE, and
use it unconditionally.  With this in place, zero allocation length is
handled simply by checking the return address of transport_kmap_data_sg.

Testcase: sg_raw /dev/sdb 12 00 83 00 00 00
    should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense
    does not fail without the patch
    fails correctly with the series

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_spc.c |   31 ++++++-------------------------
 1 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index b905fb2..647e51b 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -600,30 +600,11 @@ static int spc_emulate_inquiry(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_portal_group *tpg = cmd->se_lun->lun_sep->sep_tpg;
-	unsigned char *buf, *map_buf;
+	unsigned char *rbuf;
 	unsigned char *cdb = cmd->t_task_cdb;
+	unsigned char buf[SE_INQUIRY_BUF];
 	int p, ret;
 
-	map_buf = transport_kmap_data_sg(cmd);
-	/*
-	 * If SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is not set, then we
-	 * know we actually allocated a full page.  Otherwise, if the
-	 * data buffer is too small, allocate a temporary buffer so we
-	 * don't have to worry about overruns in all our INQUIRY
-	 * emulation handling.
-	 */
-	if (cmd->data_length < SE_INQUIRY_BUF &&
-	    (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
-		buf = kzalloc(SE_INQUIRY_BUF, GFP_KERNEL);
-		if (!buf) {
-			transport_kunmap_data_sg(cmd);
-			cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-			return -ENOMEM;
-		}
-	} else {
-		buf = map_buf;
-	}
-
 	if (dev == tpg->tpg_virt_lun0.lun_se_dev)
 		buf[0] = 0x3f; /* Not connected */
 	else
@@ -655,11 +636,11 @@ static int spc_emulate_inquiry(struct se_cmd *cmd)
 	ret = -EINVAL;
 
 out:
-	if (buf != map_buf) {
-		memcpy(map_buf, buf, cmd->data_length);
-		kfree(buf);
+	rbuf = transport_kmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
+		transport_kunmap_data_sg(cmd);
 	}
-	transport_kunmap_data_sg(cmd);
 
 	if (!ret)
 		target_complete_cmd(cmd, GOOD);
-- 
1.7.1

[PATCH 08/11] target: fix truncation of mode data, support zero allocation length

[Paolo Bonzini]

The offset was not bumped back to the full size after writing the
header of the MODE SENSE response, so the last 1 or 2 bytes were
not copied.

On top of this, support zero-length requests by checking for the
return value of transport_kmap_data_sg.

Testcase: sg_raw -r20 /dev/sdb 5a 00 0a 00 00 00 00 00 14 00
    last byte should be 0x1e
    it is 0x00 without the patch
    it is correct with the patch

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_spc.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 647e51b..96e5c48 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -784,7 +784,7 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
 	unsigned char *rbuf;
 	int type = dev->transport->get_device_type(dev);
 	int ten = (cmd->t_task_cdb[0] == MODE_SENSE_10);
-	int offset = ten ? 8 : 4;
+	u32 offset = ten ? 8 : 4;
 	int length = 0;
 	unsigned char buf[SE_MODE_PAGE_BUF];
 
@@ -817,6 +817,7 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
 		offset -= 2;
 		buf[0] = (offset >> 8) & 0xff;
 		buf[1] = offset & 0xff;
+		offset += 2;
 
 		if ((cmd->se_lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) ||
 		    (cmd->se_deve &&
@@ -826,13 +827,10 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
 		if ((dev->se_sub_dev->se_dev_attrib.emulate_write_cache > 0) &&
 		    (dev->se_sub_dev->se_dev_attrib.emulate_fua_write > 0))
 			spc_modesense_dpofua(&buf[3], type);
-
-		if ((offset + 2) > cmd->data_length)
-			offset = cmd->data_length;
-
 	} else {
 		offset -= 1;
 		buf[0] = offset & 0xff;
+		offset += 1;
 
 		if ((cmd->se_lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) ||
 		    (cmd->se_deve &&
@@ -842,14 +840,13 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
 		if ((dev->se_sub_dev->se_dev_attrib.emulate_write_cache > 0) &&
 		    (dev->se_sub_dev->se_dev_attrib.emulate_fua_write > 0))
 			spc_modesense_dpofua(&buf[2], type);
-
-		if ((offset + 1) > cmd->data_length)
-			offset = cmd->data_length;
 	}
 
 	rbuf = transport_kmap_data_sg(cmd);
-	memcpy(rbuf, buf, offset);
-	transport_kunmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, min(offset, cmd->data_length));
+		transport_kunmap_data_sg(cmd);
+	}
 
 	target_complete_cmd(cmd, GOOD);
 	return 0;
-- 
1.7.1

[PATCH 09/11] target: support zero allocation length in SBC commands

[Paolo Bonzini]

READ CAPACITY must be subject to the same treatment as INQUIRY,
REQUEST SENSE, and MODE SENSE, but there are no pre-existing bugs
to fix here.  Just use an on-stack buffer, and copy to it after
checking the return value of transport_kmap_data_sg.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_sbc.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a9dd946..868f8aa 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -40,8 +40,9 @@
 static int sbc_emulate_readcapacity(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
-	unsigned char *buf;
 	unsigned long long blocks_long = dev->transport->get_blocks(dev);
+	unsigned char *rbuf;
+	unsigned char buf[8];
 	u32 blocks;
 
 	if (blocks_long >= 0x00000000ffffffff)
@@ -49,8 +50,6 @@ static int sbc_emulate_readcapacity(struct se_cmd *cmd)
 	else
 		blocks = (u32)blocks_long;
 
-	buf = transport_kmap_data_sg(cmd);
-
 	buf[0] = (blocks >> 24) & 0xff;
 	buf[1] = (blocks >> 16) & 0xff;
 	buf[2] = (blocks >> 8) & 0xff;
@@ -60,7 +59,11 @@ static int sbc_emulate_readcapacity(struct se_cmd *cmd)
 	buf[6] = (dev->se_sub_dev->se_dev_attrib.block_size >> 8) & 0xff;
 	buf[7] = dev->se_sub_dev->se_dev_attrib.block_size & 0xff;
 
-	transport_kunmap_data_sg(cmd);
+	rbuf = transport_kmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
+		transport_kunmap_data_sg(cmd);
+	}
 
 	target_complete_cmd(cmd, GOOD);
 	return 0;
@@ -69,11 +72,11 @@ static int sbc_emulate_readcapacity(struct se_cmd *cmd)
 static int sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
-	unsigned char *buf;
+	unsigned char *rbuf;
+	unsigned char buf[32];
 	unsigned long long blocks = dev->transport->get_blocks(dev);
 
-	buf = transport_kmap_data_sg(cmd);
-
+	memset(buf, 0, sizeof(buf));
 	buf[0] = (blocks >> 56) & 0xff;
 	buf[1] = (blocks >> 48) & 0xff;
 	buf[2] = (blocks >> 40) & 0xff;
@@ -93,7 +96,11 @@ static int sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 	if (dev->se_sub_dev->se_dev_attrib.emulate_tpu || dev->se_sub_dev->se_dev_attrib.emulate_tpws)
 		buf[14] = 0x80;
 
-	transport_kunmap_data_sg(cmd);
+	rbuf = transport_kmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
+		transport_kunmap_data_sg(cmd);
+	}
 
 	target_complete_cmd(cmd, GOOD);
 	return 0;
-- 
1.7.1

[PATCH 10/11] target: do not submit a zero-bio I/O request

[Paolo Bonzini]

scsi_setup_fs_cmnd does not like to receive requests with no
bios attached to it.  Special-case zero-length reads and writes,
by not submitting any bio.

Testcase: sg_raw /dev/sdb 28 00 00 00 00 00 00 00 00 00
    should not fail
    panics with the rest of the series but not this patch
    behaves correctly without or with this series

---
 drivers/target/target_core_iblock.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 9ba4954..167c5e1 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -657,6 +657,12 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 		goto fail;
 	cmd->priv = ibr;
 
+	if (!sgl_nents) {
+		atomic_set(&ibr->pending, 1);
+		iblock_complete_cmd(cmd);
+		return 0;
+	}
+
 	bio = iblock_get_bio(cmd, block_lba, sgl_nents);
 	if (!bio)
 		goto fail_free_ibr;
-- 
1.7.1

[PATCH 11/11] target: go through normal processing for all zero-length commands

[Paolo Bonzini]

Yay, all users of transport_kmap_data_sg now check for a zero-length
request and/or a too-small parameter list length.  We can thus go through
the normal emulation path even for such commands.

This means that out-of-bounds reads and writes are now reported correctly
even if they transfer 0 blocks.  Other errors are also reported correctly.

Testcase: sg_raw /dev/sdb 28 00 80 00 00 00 00 00 00 00
    should fail with ILLEGAL REQUEST / LBA OUT OF RANGE sense
    does not fail without the patch
    (still wrong with the patch, but better: the ASC is INVALID FIELD IN CDB)

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/target/target_core_transport.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 09d9279..c071d0b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2295,24 +2295,6 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
 		if (ret < 0)
 			goto out_fail;
 	}
-	/*
-	 * If this command doesn't have any payload and we don't have to call
-	 * into the fabric for data transfers, go ahead and complete it right
-	 * away.
-	 */
-	if (!cmd->data_length &&
-	    cmd->t_task_cdb[0] != REQUEST_SENSE &&
-	    (cmd->se_dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV ||
-	     cmd->t_task_cdb[0] == REPORT_LUNS) {
-		spin_lock_irq(&cmd->t_state_lock);
-		cmd->t_state = TRANSPORT_COMPLETE;
-		cmd->transport_state |= CMD_T_ACTIVE;
-		spin_unlock_irq(&cmd->t_state_lock);
-
-		INIT_WORK(&cmd->work, target_complete_ok_work);
-		queue_work(target_completion_wq, &cmd->work);
-		return 0;
-	}
 
 	atomic_inc(&cmd->t_fe_count);
 
-- 
1.7.1

Re: [PATCH 00/11] lots of fixes for zero allocation length

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 17:30 +0200, Paolo Bonzini wrote:
> Here are the fixes for zero-length commands, that ultimately let
> all command go through normal processing, even if they have
> zero length.
> 
> The first patch is for PSCSI, the others are for IBLOCK and friends.
> I tried to order them so that the safest come first.  So for 3.6 you
> could choose to stop at patch 1, 2, 3 or 6, or not apply anything
> at all.
> 
> Testcases included in each patch, when relevant.


Very nice, so the remaining patches #7-10 have been applied into
for-next, and I'll apply a slightly modified patch #11 next week after
rebasing for-next as patches #1-6 hit -rc5 -> -rc6 along with the other
pending fixes.

Great work Paolo !!

--nab

> Paolo
> 
> Paolo Bonzini (11):
>   target: go through normal processing for zero-length PSCSI commands
>   target: report too-small parameter lists everywhere
>   target: fail REPORT LUNS with less than 16 bytes of payload
>   target: support zero-size allocation lengths in transport_kmap_data_sg
>   target: support zero allocation length in REQUEST SENSE
>   target: go through normal processing for zero-length REQUEST_SENSE
>   target: support zero allocation length in INQUIRY
>   target: fix truncation of mode data, support zero allocation length
>   target: support zero allocation length in SBC commands
>   target: do not submit a zero-bio I/O request
>   target: go through normal processing for all zero-length commands
> 
>  drivers/target/target_core_alua.c      |    7 +++
>  drivers/target/target_core_device.c    |    7 +++
>  drivers/target/target_core_iblock.c    |   17 ++++++-
>  drivers/target/target_core_pr.c        |    8 +++
>  drivers/target/target_core_pscsi.c     |    8 ++--
>  drivers/target/target_core_sbc.c       |   23 ++++++---
>  drivers/target/target_core_spc.c       |   81 +++++++++++--------------------
>  drivers/target/target_core_transport.c |   35 ++++----------
>  include/target/target_core_base.h      |    1 +
>  9 files changed, 95 insertions(+), 92 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] lots of fixes for zero allocation length

[Christoph Hellwig]

On Fri, Sep 07, 2012 at 05:30:31PM +0200, Paolo Bonzini wrote:
> Here are the fixes for zero-length commands, that ultimately let
> all command go through normal processing, even if they have
> zero length.
> 
> The first patch is for PSCSI, the others are for IBLOCK and friends.
> I tried to order them so that the safest come first.  So for 3.6 you
> could choose to stop at patch 1, 2, 3 or 6, or not apply anything
> at all.
> 
> Testcases included in each patch, when relevant.

Can you please submit them for the scsi testsuite?

Re: [PATCH 00/11] lots of fixes for zero allocation length

[Paolo Bonzini]

Il 10/09/2012 09:28, Christoph Hellwig ha scritto:
>> > Testcases included in each patch, when relevant.
> Can you please submit them for the scsi testsuite?
> 

Yes, will do.

Paolo