[12/36] target: Properly set Sense data length when copy sense

The transport_get_sense_buffer() sets sense data length to be the
allocated sense buffer length TRANSPORT_SENSE_BUFFER. However, the sense
data length is depending on the sense data description. Check the sense
data for cmd->scsi_sense_length.

See SPC4-R37 section 4.5.2.1.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/target/target_core_transport.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Hi Thinh,
On Wed, Jul 06, 2022 at 04:35:38PM -0700, Thinh Nguyen wrote:
> The transport_get_sense_buffer() sets sense data length to be the
> allocated sense buffer length TRANSPORT_SENSE_BUFFER. However, the sense
> data length is depending on the sense data description. Check the sense
> data for cmd->scsi_sense_length.
> 
> See SPC4-R37 section 4.5.2.1.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/target/target_core_transport.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 9734952a6228..2fc4ba1814c6 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -771,8 +771,6 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
>  	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION)
>  		return NULL;
>  
> -	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> -
>  	pr_debug("HBA_[%u]_PLUG[%s]: Requesting sense for SAM STATUS: 0x%02x\n",
>  		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
>  	return cmd->sense_buffer;
> @@ -791,7 +789,12 @@ void transport_copy_sense_to_cmd(struct se_cmd *cmd, unsigned char *sense)
>  	}
>  
>  	cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> -	memcpy(cmd_sense_buf, sense, cmd->scsi_sense_length);
> +	memcpy(cmd_sense_buf, sense, TRANSPORT_SENSE_BUFFER);
> +
> +	/* Sense data length = min sense data + additional sense data length */
> +	cmd->scsi_sense_length = min_t(int, 8 + cmd_sense_buf[7],
> +				       TRANSPORT_SENSE_BUFFER);
Looks a bit strange - to copy big amount of data and then to reduce the
length. Better vice versa - set correct length and copy only that data. 
> +
>  	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  }
>  EXPORT_SYMBOL(transport_copy_sense_to_cmd);

On 7/7/2022, Dmitry Bogdanov wrote:
> Hi Thinh,
> On Wed, Jul 06, 2022 at 04:35:38PM -0700, Thinh Nguyen wrote:
>> The transport_get_sense_buffer() sets sense data length to be the
>> allocated sense buffer length TRANSPORT_SENSE_BUFFER. However, the sense
>> data length is depending on the sense data description. Check the sense
>> data for cmd->scsi_sense_length.
>>
>> See SPC4-R37 section 4.5.2.1.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>   drivers/target/target_core_transport.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 9734952a6228..2fc4ba1814c6 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -771,8 +771,6 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
>>   	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION)
>>   		return NULL;
>>   
>> -	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
>> -
>>   	pr_debug("HBA_[%u]_PLUG[%s]: Requesting sense for SAM STATUS: 0x%02x\n",
>>   		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
>>   	return cmd->sense_buffer;
>> @@ -791,7 +789,12 @@ void transport_copy_sense_to_cmd(struct se_cmd *cmd, unsigned char *sense)
>>   	}
>>   
>>   	cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>> -	memcpy(cmd_sense_buf, sense, cmd->scsi_sense_length);
>> +	memcpy(cmd_sense_buf, sense, TRANSPORT_SENSE_BUFFER);
>> +
>> +	/* Sense data length = min sense data + additional sense data length */
>> +	cmd->scsi_sense_length = min_t(int, 8 + cmd_sense_buf[7],
>> +				       TRANSPORT_SENSE_BUFFER);
> Looks a bit strange - to copy big amount of data and then to reduce the
> length. Better vice versa - set correct length and copy only that data.

Sure, I'll do that.

Thanks,
Thinh

>> +
>>   	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>>   }
>>   EXPORT_SYMBOL(transport_copy_sense_to_cmd);