[PATCH 3.6 0/2] More PSCSI fixes

[PATCH 3.6 0/2] More PSCSI fixes

[Paolo Bonzini]

Hi all,

here are two more fixes for SCSI passthrough.  The first removes an
(IMO) ill-conceived functionality and fixes passthrough of protection
information.  The second started as a fix for START STOP UNIT, and ended
up as a more general change in transport_kmap_data_sg.

Please review!

Paolo

Paolo Bonzini (2):
  target: remove pscsi_clear_cdb_lun
  target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

 drivers/target/target_core_pscsi.c     |   26 -------------
 drivers/target/target_core_transport.c |   62 ++++++++++++++-----------------
 2 files changed, 28 insertions(+), 60 deletions(-)

[PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Paolo Bonzini]

The purpose of this function is to clear a LUN set in the CDB, in case
the initiator talking to us is speaking an old standards version.
However, as things stand, pscsi_clear_cdb_lun has two problems.  It
will "deceive" the guest by clearing the LUN bits on initial
commands (INQUIRY, TEST UNIT READY, etc.); but then it will let the
LUN bits through on reads, which will likely fail due to protection not
being enabled.  Second, not all commands are properly filtered, in
particular WRITE's WRPROTECT bits are cleared.

This should be done by the fabric module rather than by PSCSI, if it
knows such initiators may be lying around _and_ it can assume that its
initiators do not really care about protection information.  Nuke it
from PSCSI.

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

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 5f7151d..a0fb2c3 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1031,30 +1031,6 @@ fail:
 	return -ENOMEM;
 }
 
-/*
- * Clear a lun set in the cdb if the initiator talking to use spoke
- * and old standards version, as we can't assume the underlying device
- * won't choke up on it.
- */
-static inline void pscsi_clear_cdb_lun(unsigned char *cdb)
-{
-	switch (cdb[0]) {
-	case READ_10: /* SBC - RDProtect */
-	case READ_12: /* SBC - RDProtect */
-	case READ_16: /* SBC - RDProtect */
-	case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
-	case VERIFY: /* SBC - VRProtect */
-	case VERIFY_16: /* SBC - VRProtect */
-	case WRITE_VERIFY: /* SBC - VRProtect */
-	case WRITE_VERIFY_12: /* SBC - VRProtect */
-	case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
-		break;
-	default:
-		cdb[1] &= 0x1f; /* clear logical unit number */
-		break;
-	}
-}
-
 static int pscsi_parse_cdb(struct se_cmd *cmd)
 {
 	unsigned char *cdb = cmd->t_task_cdb;
@@ -1067,8 +1043,6 @@ static int pscsi_parse_cdb(struct se_cmd *cmd)
 		return -EINVAL;
 	}
 
-	pscsi_clear_cdb_lun(cdb);
-
 	/*
 	 * For REPORT LUNS we always need to emulate the response, for everything
 	 * else the default for pSCSI is to pass the command to the underlying
-- 
1.7.1

Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Nicholas A. Bellinger]

On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
> The purpose of this function is to clear a LUN set in the CDB, in case
> the initiator talking to us is speaking an old standards version.
> However, as things stand, pscsi_clear_cdb_lun has two problems.  It
> will "deceive" the guest by clearing the LUN bits on initial
> commands (INQUIRY, TEST UNIT READY, etc.); but then it will let the
> LUN bits through on reads, which will likely fail due to protection not
> being enabled.  Second, not all commands are properly filtered, in
> particular WRITE's WRPROTECT bits are cleared.
> 
> This should be done by the fabric module rather than by PSCSI, if it
> knows such initiators may be lying around _and_ it can assume that its
> initiators do not really care about protection information.  Nuke it
> from PSCSI.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

NAK.  This code was originally added to prevent certain pSCSI HBAs from
going bonkers when they got a legacy SCSI LUN ID encoded within the CDB
(eg: the fabric LUN ID) that's different from the physical SCSI LUN ID
on an Parallel SCSI bus.

If there are more special cases where the legacy SCSI LUN ID bit-range
needs to be cleared than please add those missing bits, but I don't
think it's yet safe to remove this completely for pSCSI w/ older LLDs.

>  drivers/target/target_core_pscsi.c |   26 --------------------------
>  1 files changed, 0 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index 5f7151d..a0fb2c3 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -1031,30 +1031,6 @@ fail:
>  	return -ENOMEM;
>  }
>  
> -/*
> - * Clear a lun set in the cdb if the initiator talking to use spoke
> - * and old standards version, as we can't assume the underlying device
> - * won't choke up on it.
> - */
> -static inline void pscsi_clear_cdb_lun(unsigned char *cdb)
> -{
> -	switch (cdb[0]) {
> -	case READ_10: /* SBC - RDProtect */
> -	case READ_12: /* SBC - RDProtect */
> -	case READ_16: /* SBC - RDProtect */
> -	case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
> -	case VERIFY: /* SBC - VRProtect */
> -	case VERIFY_16: /* SBC - VRProtect */
> -	case WRITE_VERIFY: /* SBC - VRProtect */
> -	case WRITE_VERIFY_12: /* SBC - VRProtect */
> -	case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
> -		break;
> -	default:
> -		cdb[1] &= 0x1f; /* clear logical unit number */
> -		break;
> -	}
> -}
> -
>  static int pscsi_parse_cdb(struct se_cmd *cmd)
>  {
>  	unsigned char *cdb = cmd->t_task_cdb;
> @@ -1067,8 +1043,6 @@ static int pscsi_parse_cdb(struct se_cmd *cmd)
>  		return -EINVAL;
>  	}
>  
> -	pscsi_clear_cdb_lun(cdb);
> -
>  	/*
>  	 * For REPORT LUNS we always need to emulate the response, for everything
>  	 * else the default for pSCSI is to pass the command to the underlying

Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Paolo Bonzini]

Il 06/09/2012 20:58, Nicholas A. Bellinger ha scritto:
> On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
>> The purpose of this function is to clear a LUN set in the CDB, in case
>> the initiator talking to us is speaking an old standards version.
>> However, as things stand, pscsi_clear_cdb_lun has two problems.  It
>> will "deceive" the guest by clearing the LUN bits on initial
>> commands (INQUIRY, TEST UNIT READY, etc.); but then it will let the
>> LUN bits through on reads, which will likely fail due to protection not
>> being enabled.  Second, not all commands are properly filtered, in
>> particular WRITE's WRPROTECT bits are cleared.
>>
>> This should be done by the fabric module rather than by PSCSI, if it
>> knows such initiators may be lying around _and_ it can assume that its
>> initiators do not really care about protection information.  Nuke it
>> from PSCSI.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
> 
> NAK.  This code was originally added to prevent certain pSCSI HBAs from
> going bonkers when they got a legacy SCSI LUN ID encoded within the CDB
> (eg: the fabric LUN ID) that's different from the physical SCSI LUN ID
> on an Parallel SCSI bus.

I understood, but what's good in making INQUIRY work, if the HBA will
equally go bonkers on the first actual I/O (READ/WRITE/VERIFY are all
affected)?

This is something the LLDs or the fabric module should be doing.  But I
guess it could be an attribute too.

Paolo

> If there are more special cases where the legacy SCSI LUN ID bit-range
> needs to be cleared than please add those missing bits, but I don't
> think it's yet safe to remove this completely for pSCSI w/ older LLDs.
> 
>>  drivers/target/target_core_pscsi.c |   26 --------------------------
>>  1 files changed, 0 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
>> index 5f7151d..a0fb2c3 100644
>> --- a/drivers/target/target_core_pscsi.c
>> +++ b/drivers/target/target_core_pscsi.c
>> @@ -1031,30 +1031,6 @@ fail:
>>  	return -ENOMEM;
>>  }
>>  
>> -/*
>> - * Clear a lun set in the cdb if the initiator talking to use spoke
>> - * and old standards version, as we can't assume the underlying device
>> - * won't choke up on it.
>> - */
>> -static inline void pscsi_clear_cdb_lun(unsigned char *cdb)
>> -{
>> -	switch (cdb[0]) {
>> -	case READ_10: /* SBC - RDProtect */
>> -	case READ_12: /* SBC - RDProtect */
>> -	case READ_16: /* SBC - RDProtect */
>> -	case SEND_DIAGNOSTIC: /* SPC - SELF-TEST Code */
>> -	case VERIFY: /* SBC - VRProtect */
>> -	case VERIFY_16: /* SBC - VRProtect */
>> -	case WRITE_VERIFY: /* SBC - VRProtect */
>> -	case WRITE_VERIFY_12: /* SBC - VRProtect */
>> -	case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
>> -		break;
>> -	default:
>> -		cdb[1] &= 0x1f; /* clear logical unit number */
>> -		break;
>> -	}
>> -}
>> -
>>  static int pscsi_parse_cdb(struct se_cmd *cmd)
>>  {
>>  	unsigned char *cdb = cmd->t_task_cdb;
>> @@ -1067,8 +1043,6 @@ static int pscsi_parse_cdb(struct se_cmd *cmd)
>>  		return -EINVAL;
>>  	}
>>  
>> -	pscsi_clear_cdb_lun(cdb);
>> -
>>  	/*
>>  	 * For REPORT LUNS we always need to emulate the response, for everything
>>  	 * else the default for pSCSI is to pass the command to the underlying
> 
> 

Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Nicholas A. Bellinger]

On Thu, 2012-09-06 at 22:51 +0200, Paolo Bonzini wrote:
> Il 06/09/2012 20:58, Nicholas A. Bellinger ha scritto:
> > On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
> >> The purpose of this function is to clear a LUN set in the CDB, in case
> >> the initiator talking to us is speaking an old standards version.
> >> However, as things stand, pscsi_clear_cdb_lun has two problems.  It
> >> will "deceive" the guest by clearing the LUN bits on initial
> >> commands (INQUIRY, TEST UNIT READY, etc.); but then it will let the
> >> LUN bits through on reads, which will likely fail due to protection not
> >> being enabled.  Second, not all commands are properly filtered, in
> >> particular WRITE's WRPROTECT bits are cleared.
> >>
> >> This should be done by the fabric module rather than by PSCSI, if it
> >> knows such initiators may be lying around _and_ it can assume that its
> >> initiators do not really care about protection information.  Nuke it
> >> from PSCSI.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> > 
> > NAK.  This code was originally added to prevent certain pSCSI HBAs from
> > going bonkers when they got a legacy SCSI LUN ID encoded within the CDB
> > (eg: the fabric LUN ID) that's different from the physical SCSI LUN ID
> > on an Parallel SCSI bus.
> 
> I understood, but what's good in making INQUIRY work, if the HBA will
> equally go bonkers on the first actual I/O (READ/WRITE/VERIFY are all
> affected)?
> 

The original purpose of pscsi_clear_cdb_lun() is to clear byte 1 bit 0-5
for all incoming CDBs in SCSI-2 that did encode LUN ID into the CDB as a
work-around for (some) Parallel SCSI hardware that depends on this value
to function.

As modern SCSI-3 + SAS fabrics, et al have moved away from doing this
completely, thinking about this some more I would be OK with disabling
this by default for modern SCSI HW if the pSCSI device is reporting >=
SCSI-3 from se_subsystem_api->get_device_rev().

> This is something the LLDs or the fabric module should be doing.  But I
> guess it could be an attribute too.
> 

I think it's safe to disable this for modern SCSI hw, but lets keep it
around for pSCSI backend devices that are reporting SCSI-2 support and
below.

Care to respin..?  ;)

Thanks Paolo!

Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Paolo Bonzini]

Il 07/09/2012 05:22, Nicholas A. Bellinger ha scritto:
> Care to respin..?  ;)

Sure, but I suppose this will be 3.7 material.  Do you want an attribute
too?

Paolo

Re: [PATCH 3.6 1/2] target: remove pscsi_clear_cdb_lun

[Nicholas A. Bellinger]

On Fri, 2012-09-07 at 13:51 +0200, Paolo Bonzini wrote:
> Il 07/09/2012 05:22, Nicholas A. Bellinger ha scritto:
> > Care to respin..?  ;)
> 
> Sure, but I suppose this will be 3.7 material.

<nod>

> Do you want an attribute too?
> 

Mmmmm, still undecided on that one.  I'd really like to avoid adding
pSCSI specific backend attributes, so lets try first try disabling this
logic for >= SCSI_3 devices and if folks run into problems we can
consider smarter detection (say via some simple domain validation tests
with pSCSI devices) or if it's really un-avoidable an new backend
attribute to handle this for old hardware.

Thanks Paolo!

[PATCH 3.6 2/2] target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

[Paolo Bonzini]

This patch started with the aim of fixing START STOP UNIT to a PSCSI
device.  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.  As a hint of this, we have a hack to
clear a unit attention state on a zero-size REQUEST SENSE.

The problem with fixing this, is that we do not have a page to pass
back to the caller of transport_kmap_data_sg.  But this is just
a special case of a more general overflow that could happen after
using transport_kmap_data_sg.  For example, the REQUEST_SENSE handler
expects 8 bytes, but if you send a CDB with a small allocation length
(e.g. 4 bytes).  you might end up with a single-page sglist and a large
sg->offset.  This would make buf[7] already inaccessible.

Luckily, transport_kmap_data_sg is not called on the I/O path, so we can
simply allocate a one-page bounce buffer there, which indeed also takes
care of zero-sized transfers.
---
 drivers/target/target_core_transport.c |   62 ++++++++++++++-----------------
 1 files changed, 28 insertions(+), 34 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 09028af..a77c8aa 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2181,20 +2181,32 @@ EXPORT_SYMBOL(transport_generic_map_mem_to_cmd);
 
 void *transport_kmap_data_sg(struct se_cmd *cmd)
 {
+	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
 	struct scatterlist *sg = cmd->t_data_sg;
 	struct page **pages;
 	int i;
 
-	BUG_ON(!sg);
+	BUG_ON(!sg && npages > 0);
+
 	/*
 	 * 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
 	 * control CDBs passed as SGLs via transport_generic_map_mem_to_cmd()
+	 *
+	 * This could cause overflows if the buffer is too small for the caller.
+	 * For example, the REQUEST_SENSE handler expects 8 bytes, but it is
+	 * possible to send a CDB with a small allocation length (e.g. 4 bytes).
+	 * In this case, we could have a single-page sglist with a large offset,
+	 * so that buf[7] is already inaccessible.
+	 *
+	 * But transport_kmap_data_sg is not called on the I/O path, so we can
+	 * simply allocate a one-page bounce buffer here.  This also takes care
+	 * of the case of zero-sized transfers.
 	 */
-	if (!cmd->t_data_nents)
-		return NULL;
-	else if (cmd->t_data_nents == 1)
-		return kmap(sg_page(sg)) + sg->offset;
+	if (npages <= 1) {
+		cmd->t_data_vmap = kzalloc(PAGE_SIZE, GFP_KERNEL);
+		return cmd->t_data_vmap;
+	}
 
 	/* >1 page. use vmap */
 	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
@@ -2217,14 +2229,18 @@ EXPORT_SYMBOL(transport_kmap_data_sg);
 
 void transport_kunmap_data_sg(struct se_cmd *cmd)
 {
-	if (!cmd->t_data_nents) {
-		return;
-	} else if (cmd->t_data_nents == 1) {
-		kunmap(sg_page(cmd->t_data_sg));
-		return;
-	}
+	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
+	if (npages <= 1) {
+		if (npages) {
+			struct scatterlist *sg = cmd->t_data_sg;
+			u8 *dest = kmap(sg_page(sg));
+			memcpy(dest + sg->offset, cmd->t_data_vmap, sg->length);
+			kunmap(sg_page(sg));
+		}
+		kfree(cmd->t_data_vmap);
+	} else
+		vunmap(cmd->t_data_vmap);
 
-	vunmap(cmd->t_data_vmap);
 	cmd->t_data_vmap = NULL;
 }
 EXPORT_SYMBOL(transport_kunmap_data_sg);
@@ -2290,28 +2306,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) {
-		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);
-
-		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;
-	}
 
 	atomic_inc(&cmd->t_fe_count);
 
-- 
1.7.1

Re: [PATCH 3.6 2/2] target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

[Nicholas A. Bellinger]

On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
> This patch started with the aim of fixing START STOP UNIT to a PSCSI
> device.  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.  As a hint of this, we have a hack to
> clear a unit attention state on a zero-size REQUEST SENSE.
> 

The existing code for zero-size handling in transport_generic_new_cmd()
is correct for virtual backends (eg: TRANSPORT_PLUGIN_VHBA_*), but as
you've witnessed not correct for pSCSI passthrough ops.

> 
> 
> The problem with fixing this, is that we do not have a page to pass
> back to the caller of transport_kmap_data_sg.  But this is just
> a special case of a more general overflow that could happen after
> using transport_kmap_data_sg.

>   For example, the REQUEST_SENSE handler
> expects 8 bytes, but if you send a CDB with a small allocation length
> (e.g. 4 bytes).  you might end up with a single-page sglist and a large
> sg->offset.  This would make buf[7] already inaccessible.
> 

In existing code for TRANSPORT_PLUGIN_VHBA_* backends, there are no
cases where transport_kmap_data_sg() is ever called with a zero-sized
CDBs.

The only cases where an pSCSI backend ever uses transport_kmap_data_sg()
today are:

- During REPORT_LUNS emulation

- During the MODE_SENSE hack in pscsi_transport_complete() to set the
  proper WriteProtected bit based upon configfs fabric attribute

so I'd rather see two special case checks for zero-size CDBs with pSCSI,
over adding these changes to transport_k[un]map_data_sg().

> Luckily, transport_kmap_data_sg is not called on the I/O path, so we can
> simply allocate a one-page bounce buffer there, which indeed also takes
> care of zero-sized transfers.
> ---
>  drivers/target/target_core_transport.c |   62 ++++++++++++++-----------------
>  1 files changed, 28 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 09028af..a77c8aa 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2181,20 +2181,32 @@ EXPORT_SYMBOL(transport_generic_map_mem_to_cmd);
>  
>  void *transport_kmap_data_sg(struct se_cmd *cmd)
>  {
> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
>  	struct scatterlist *sg = cmd->t_data_sg;
>  	struct page **pages;
>  	int i;
>  
> -	BUG_ON(!sg);
> +	BUG_ON(!sg && npages > 0);
> +
>  	/*
>  	 * 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
>  	 * control CDBs passed as SGLs via transport_generic_map_mem_to_cmd()
> +	 *
> +	 * This could cause overflows if the buffer is too small for the caller.
> +	 * For example, the REQUEST_SENSE handler expects 8 bytes, but it is
> +	 * possible to send a CDB with a small allocation length (e.g. 4 bytes).
> +	 * In this case, we could have a single-page sglist with a large offset,
> +	 * so that buf[7] is already inaccessible.
> +	 *
> +	 * But transport_kmap_data_sg is not called on the I/O path, so we can
> +	 * simply allocate a one-page bounce buffer here.  This also takes care
> +	 * of the case of zero-sized transfers.
>  	 */
> -	if (!cmd->t_data_nents)
> -		return NULL;
> -	else if (cmd->t_data_nents == 1)
> -		return kmap(sg_page(sg)) + sg->offset;
> +	if (npages <= 1) {
> +		cmd->t_data_vmap = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +		return cmd->t_data_vmap;
> +	}
>  
>  	/* >1 page. use vmap */
>  	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
> @@ -2217,14 +2229,18 @@ EXPORT_SYMBOL(transport_kmap_data_sg);
>  
>  void transport_kunmap_data_sg(struct se_cmd *cmd)
>  {
> -	if (!cmd->t_data_nents) {
> -		return;
> -	} else if (cmd->t_data_nents == 1) {
> -		kunmap(sg_page(cmd->t_data_sg));
> -		return;
> -	}
> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
> +	if (npages <= 1) {
> +		if (npages) {
> +			struct scatterlist *sg = cmd->t_data_sg;
> +			u8 *dest = kmap(sg_page(sg));
> +			memcpy(dest + sg->offset, cmd->t_data_vmap, sg->length);
> +			kunmap(sg_page(sg));
> +		}
> +		kfree(cmd->t_data_vmap);
> +	} else
> +		vunmap(cmd->t_data_vmap);
>  
> -	vunmap(cmd->t_data_vmap);
>  	cmd->t_data_vmap = NULL;
>  }
>  EXPORT_SYMBOL(transport_kunmap_data_sg);
> @@ -2290,28 +2306,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) {
> -		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);
> -
> -		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;
> -	}
>  

This code needs still needs to get called for all virtual backends of
type !TRANSPORT_PLUGIN_PHBA_PDEV.

Re: [PATCH 3.6 2/2] target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

[Paolo Bonzini]

Il 06/09/2012 21:29, Nicholas A. Bellinger ha scritto:
> On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
>> This patch started with the aim of fixing START STOP UNIT to a PSCSI
>> device.  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.  As a hint of this, we have a hack to
>> clear a unit attention state on a zero-size REQUEST SENSE.
> 
> The existing code for zero-size handling in transport_generic_new_cmd()
> is correct for virtual backends (eg: TRANSPORT_PLUGIN_VHBA_*), but as
> you've witnessed not correct for pSCSI passthrough ops.

It is not completely correct for virtual backends, for example I think
it will always return success for 0-block reads or writes, even if the
start LBA is out of range.  This is also something that I saw with
PSCSI, and is fixed by these patches.

Also, even though it handles zero-size, it doesn't handle a CDB with a
small but nonzero allocation length.  If you have such a CDB, you can
overflow the sglist.   If the fabric uses
transport_generic_map_mem_to_cmd, you may corrupt adjacent memory.

Besides, the cut-and-pasted REQUEST SENSE handling is a bit gross. :)

Fixing this properly, as it turns out, also fixes zero-size handling of
PSCSI.

> The only cases where an pSCSI backend ever uses transport_kmap_data_sg()
> today are:
> 
> - During REPORT_LUNS emulation
> 
> - During the MODE_SENSE hack in pscsi_transport_complete() to set the
>   proper WriteProtected bit based upon configfs fabric attribute
> 
> so I'd rather see two special case checks for zero-size CDBs with pSCSI,
> over adding these changes to transport_k[un]map_data_sg().

This would not fix the root cause, which is bad handling of short-sized
allocation lengths.

>> Luckily, transport_kmap_data_sg is not called on the I/O path, so we can
>> simply allocate a one-page bounce buffer there, which indeed also takes
>> care of zero-sized transfers.
>> ---
>>  drivers/target/target_core_transport.c |   62 ++++++++++++++-----------------
>>  1 files changed, 28 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 09028af..a77c8aa 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -2181,20 +2181,32 @@ EXPORT_SYMBOL(transport_generic_map_mem_to_cmd);
>>  
>>  void *transport_kmap_data_sg(struct se_cmd *cmd)
>>  {
>> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
>>  	struct scatterlist *sg = cmd->t_data_sg;
>>  	struct page **pages;
>>  	int i;
>>  
>> -	BUG_ON(!sg);
>> +	BUG_ON(!sg && npages > 0);
>> +
>>  	/*
>>  	 * 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
>>  	 * control CDBs passed as SGLs via transport_generic_map_mem_to_cmd()
>> +	 *
>> +	 * This could cause overflows if the buffer is too small for the caller.
>> +	 * For example, the REQUEST_SENSE handler expects 8 bytes, but it is
>> +	 * possible to send a CDB with a small allocation length (e.g. 4 bytes).
>> +	 * In this case, we could have a single-page sglist with a large offset,
>> +	 * so that buf[7] is already inaccessible.
>> +	 *
>> +	 * But transport_kmap_data_sg is not called on the I/O path, so we can
>> +	 * simply allocate a one-page bounce buffer here.  This also takes care
>> +	 * of the case of zero-sized transfers.
>>  	 */
>> -	if (!cmd->t_data_nents)
>> -		return NULL;
>> -	else if (cmd->t_data_nents == 1)
>> -		return kmap(sg_page(sg)) + sg->offset;
>> +	if (npages <= 1) {
>> +		cmd->t_data_vmap = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +		return cmd->t_data_vmap;
>> +	}
>>  
>>  	/* >1 page. use vmap */
>>  	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
>> @@ -2217,14 +2229,18 @@ EXPORT_SYMBOL(transport_kmap_data_sg);
>>  
>>  void transport_kunmap_data_sg(struct se_cmd *cmd)
>>  {
>> -	if (!cmd->t_data_nents) {
>> -		return;
>> -	} else if (cmd->t_data_nents == 1) {
>> -		kunmap(sg_page(cmd->t_data_sg));
>> -		return;
>> -	}
>> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
>> +	if (npages <= 1) {
>> +		if (npages) {
>> +			struct scatterlist *sg = cmd->t_data_sg;
>> +			u8 *dest = kmap(sg_page(sg));
>> +			memcpy(dest + sg->offset, cmd->t_data_vmap, sg->length);
>> +			kunmap(sg_page(sg));
>> +		}
>> +		kfree(cmd->t_data_vmap);
>> +	} else
>> +		vunmap(cmd->t_data_vmap);
>>  
>> -	vunmap(cmd->t_data_vmap);
>>  	cmd->t_data_vmap = NULL;
>>  }
>>  EXPORT_SYMBOL(transport_kunmap_data_sg);
>> @@ -2290,28 +2306,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) {
>> -		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);
>> -
>> -		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;
>> -	}
>>  
> 
> This code needs still needs to get called for all virtual backends of
> type !TRANSPORT_PLUGIN_PHBA_PDEV.

It is superseded by the new code in transport_kmap_data_sg.

Paolo

Re: [PATCH 3.6 2/2] target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

[Nicholas A. Bellinger]

On Thu, 2012-09-06 at 22:58 +0200, Paolo Bonzini wrote:
> Il 06/09/2012 21:29, Nicholas A. Bellinger ha scritto:
> > On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
> >> This patch started with the aim of fixing START STOP UNIT to a PSCSI
> >> device.  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.  As a hint of this, we have a hack to
> >> clear a unit attention state on a zero-size REQUEST SENSE.
> > 
> > The existing code for zero-size handling in transport_generic_new_cmd()
> > is correct for virtual backends (eg: TRANSPORT_PLUGIN_VHBA_*), but as
> > you've witnessed not correct for pSCSI passthrough ops.
> 
> It is not completely correct for virtual backends, for example I think
> it will always return success for 0-block reads or writes, even if the
> start LBA is out of range.  This is also something that I saw with
> PSCSI, and is fixed by these patches.
> 

Good point !

> Also, even though it handles zero-size, it doesn't handle a CDB with a
> small but nonzero allocation length.  If you have such a CDB, you can
> overflow the sglist. 

Any particular sg_raw example in mind that can trigger this..?

> If the fabric uses transport_generic_map_mem_to_cmd, you may corrupt
> adjacent memory.
> 

Yes, this is the case for transport_generic_map_mem_to_cmd() consumers
(loopback + tcm_vhost) where we currently have to reject SCSI Overflow
cases because of exactly this type of scenario..

> 
> 
> Besides, the cut-and-pasted REQUEST SENSE handling is a bit gross. :)
> 

So is sending zero-length allocation lengths for control CDBs.  ;)

> Fixing this properly, as it turns out, also fixes zero-size handling of
> PSCSI.
> 

...

> > The only cases where an pSCSI backend ever uses transport_kmap_data_sg()
> > today are:
> > 
> > - During REPORT_LUNS emulation
> > 
> > - During the MODE_SENSE hack in pscsi_transport_complete() to set the
> >   proper WriteProtected bit based upon configfs fabric attribute
> > 
> > so I'd rather see two special case checks for zero-size CDBs with pSCSI,
> > over adding these changes to transport_k[un]map_data_sg().
> 
> This would not fix the root cause, which is bad handling of short-sized
> allocation lengths.
> 
> >> Luckily, transport_kmap_data_sg is not called on the I/O path, so we can
> >> simply allocate a one-page bounce buffer there, which indeed also takes
> >> care of zero-sized transfers.
> >> ---
> >>  drivers/target/target_core_transport.c |   62 ++++++++++++++-----------------
> >>  1 files changed, 28 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> >> index 09028af..a77c8aa 100644
> >> --- a/drivers/target/target_core_transport.c
> >> +++ b/drivers/target/target_core_transport.c
> >> @@ -2181,20 +2181,32 @@ EXPORT_SYMBOL(transport_generic_map_mem_to_cmd);
> >>  
> >>  void *transport_kmap_data_sg(struct se_cmd *cmd)
> >>  {
> >> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
> >>  	struct scatterlist *sg = cmd->t_data_sg;
> >>  	struct page **pages;
> >>  	int i;
> >>  
> >> -	BUG_ON(!sg);
> >> +	BUG_ON(!sg && npages > 0);
> >> +
> >>  	/*
> >>  	 * 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
> >>  	 * control CDBs passed as SGLs via transport_generic_map_mem_to_cmd()
> >> +	 *
> >> +	 * This could cause overflows if the buffer is too small for the caller.
> >> +	 * For example, the REQUEST_SENSE handler expects 8 bytes, but it is
> >> +	 * possible to send a CDB with a small allocation length (e.g. 4 bytes).
> >> +	 * In this case, we could have a single-page sglist with a large offset,
> >> +	 * so that buf[7] is already inaccessible.
> >> +	 *
> >> +	 * But transport_kmap_data_sg is not called on the I/O path, so we can
> >> +	 * simply allocate a one-page bounce buffer here.  This also takes care
> >> +	 * of the case of zero-sized transfers.
> >>  	 */
> >> -	if (!cmd->t_data_nents)
> >> -		return NULL;
> >> -	else if (cmd->t_data_nents == 1)
> >> -		return kmap(sg_page(sg)) + sg->offset;
> >> +	if (npages <= 1) {
> >> +		cmd->t_data_vmap = kzalloc(PAGE_SIZE, GFP_KERNEL);
> >> +		return cmd->t_data_vmap;
> >> +	}
> >>  
> >>  	/* >1 page. use vmap */
> >>  	pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
> >> @@ -2217,14 +2229,18 @@ EXPORT_SYMBOL(transport_kmap_data_sg);
> >>  
> >>  void transport_kunmap_data_sg(struct se_cmd *cmd)
> >>  {
> >> -	if (!cmd->t_data_nents) {
> >> -		return;
> >> -	} else if (cmd->t_data_nents == 1) {
> >> -		kunmap(sg_page(cmd->t_data_sg));
> >> -		return;
> >> -	}
> >> +	u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
> >> +	if (npages <= 1) {
> >> +		if (npages) {
> >> +			struct scatterlist *sg = cmd->t_data_sg;
> >> +			u8 *dest = kmap(sg_page(sg));
> >> +			memcpy(dest + sg->offset, cmd->t_data_vmap, sg->length);
> >> +			kunmap(sg_page(sg));
> >> +		}
> >> +		kfree(cmd->t_data_vmap);
> >> +	} else
> >> +		vunmap(cmd->t_data_vmap);
> >>  
> >> -	vunmap(cmd->t_data_vmap);
> >>  	cmd->t_data_vmap = NULL;
> >>  }
> >>  EXPORT_SYMBOL(transport_kunmap_data_sg);
> >> @@ -2290,28 +2306,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) {
> >> -		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);
> >> -
> >> -		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;
> >> -	}
> >>  
> > 
> > This code needs still needs to get called for all virtual backends of
> > type !TRANSPORT_PLUGIN_PHBA_PDEV.
> 
> It is superseded by the new code in transport_kmap_data_sg.
> 

Ok, if this is a genuine issue then please show how to trigger with
sg_raw, and let's plan on merging this as a -rc6 bugfix in order to
spend some more testing w/ scsi-testsuite across different backends over
the next week.

Thanks Paolo!

Re: [PATCH 3.6 2/2] target: use a bounce buffer in transport_kmap_data_sg for 0 or 1-page sglist

[Paolo Bonzini]

Il 07/09/2012 05:35, Nicholas A. Bellinger ha scritto:
>> It is not completely correct for virtual backends, for example I think
>> it will always return success for 0-block reads or writes, even if the
>> start LBA is out of range.  This is also something that I saw with
>> PSCSI, and is fixed by these patches.
> 
> Good point !

I spent the morning digging further down the rabbithole, and I found the
following testcases:

REPORT LUNS:
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

INQUIRY (VPD PAGE != 0, std != 0):
sg_raw /dev/sdb 12 00 83 00 00 00
    should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense
    does not fail

MODE SENSE off by one (by two for 10-byte CDB):
sg_raw -r20 /dev/sdb 5a 00 0a 00 00 00 00 00 14 00
    last byte should be 0x1e
    it is 0x00

READ:
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

Plus:

- missing checks on parameter list length for PR OUT with SPEC_I_PT,
UNMAP, SET TARGET PORT GROUPS.  In some cases these could lead to
reading undefined data.

- no checks for OOM in callers of transport_kmem_data_sg.

>> Also, even though it handles zero-size, it doesn't handle a CDB with a
>> small but nonzero allocation length.  If you have such a CDB, you can
>> overflow the sglist. 
> 
> Any particular sg_raw example in mind that can trigger this..?

sg_raw (or even SG_IO) doesn't work because misaligned scatterlists are
bounce-buffered by the block layer in blk_map_rq_user.

However, given the above bugs it's better to attack the callers of
transport_kmem_data_sg one by one.  Again, zero-length CDB support comes
for free.

> Ok, if this is a genuine issue then please show how to trigger with
> sg_raw, and let's plan on merging this as a -rc6 bugfix in order to
> spend some more testing w/ scsi-testsuite across different backends over
> the next week.

Doesn't seem to be too important, it can be done for 3.7 except perhaps
for PSCSI; I'll put the PSCSI patch at the beginning of the series.  I
can make scsi-testsuite patches for the above issues, but I'll be glad
if someone beats me to it.

Paolo