On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> Avoid that the iSCSI target driver complains about "Initial page entry
> out-of-bounds" and closes the connection if the SCSI Data-Out buffer
> is larger than the buffer size specified through the CDB. This patch
> prevents that running the libiscsi regression tests against LIO trigger
> an infinite loop of libiscsi submitting a command, LIO closing the
> connection and libiscsi resubmitting the same command.
>
This statement and patch is incorrect.
target-core has always rejected SCF_SCSI_DATA_CDB WRITEs with
overflow/underflow since day one:
>From target_cmd_size_check():
} else if (size != cmd->data_length) {
pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
" %u does not match SCSI CDB Length: %u for SAM Opcode:"
" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
cmd->data_length, size, cmd->t_task_cdb[0]);
if (cmd->data_direction == DMA_TO_DEVICE &&
cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
pr_err("Rejecting underflow/overflow WRITE data\n");
return TCM_INVALID_CDB_FIELD;
}
......
}
The reason you're suddenly hitting this now is because your patch in
target-pending/for-next:
commit 0e2eb7d12eaa8e391bf5615d4271bb87a649caaa
Author: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Date: Thu Mar 30 10:12:39 2017 -0700
incorrectly started allowing WRITE_VERIFY_* w/ bytchk = 0, without
actually setting SCF_SCSI_DATA_CDB in sbc_parse_verify():
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;
}
Patch #18 is also trying to (incorrectly) address the same problem.
I'll post the proper fix for the regression introduced by your earlier
patch there.
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Andy Grover <agrover@xxxxxxxxxx>
> Cc: David Disseldorp <ddiss@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_transport.c | 29 ++++++++++-------------------
> include/target/target_core_base.h | 4 +++-
> 2 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index df1ceb2dd110..86b6b0238975 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1159,11 +1159,11 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
>
> if (cmd->unknown_data_length) {
> cmd->data_length = size;
> - } else if (size != cmd->data_length) {
> + } else if (size != cmd->buffer_length) {
> pr_warn("TARGET_CORE[%s]: Expected Transfer Length:"
> " %u does not match SCSI CDB Length: %u for SAM Opcode:"
> " 0x%02x\n", cmd->se_tfo->get_fabric_name(),
> - cmd->data_length, size, cmd->t_task_cdb[0]);
> + cmd->buffer_length, size, cmd->t_task_cdb[0]);
>
> if (cmd->data_direction == DMA_TO_DEVICE &&
> cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
> @@ -1183,7 +1183,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
> }
> /*
> * For the overflow case keep the existing fabric provided
> - * ->data_length. Otherwise for the underflow case, reset
> + * ->buffer_length. Otherwise for the underflow case, reset
> * ->data_length to the smaller SCSI expected data transfer
> * length.
> */
> @@ -1227,6 +1227,7 @@ void transport_init_se_cmd(
>
> cmd->se_tfo = tfo;
> cmd->se_sess = se_sess;
> + cmd->buffer_length = data_length;
> cmd->data_length = data_length;
> cmd->data_direction = data_direction;
> cmd->sam_task_attr = task_attr;
> @@ -2412,41 +2413,31 @@ transport_generic_new_cmd(struct se_cmd *cmd)
> * beforehand.
> */
> if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) &&
> - cmd->data_length) {
> + cmd->buffer_length) {
>
> if ((cmd->se_cmd_flags & SCF_BIDI) ||
> (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)) {
> - u32 bidi_length;
> -
> - if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)
> - bidi_length = cmd->t_task_nolb *
> - cmd->se_dev->dev_attrib.block_size;
> - else
> - bidi_length = cmd->data_length;
> -
> ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
> &cmd->t_bidi_data_nents,
> - bidi_length, zero_flag, false);
> + cmd->buffer_length, zero_flag,
> + false);
> if (ret < 0)
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
>
> ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
> - cmd->data_length, zero_flag, false);
> + cmd->buffer_length, zero_flag, false);
> if (ret < 0)
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> } else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) &&
> - cmd->data_length) {
> + cmd->buffer_length) {
> /*
> * Special case for COMPARE_AND_WRITE with fabrics
> * using SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC.
> */
> - u32 caw_length = cmd->t_task_nolb *
> - cmd->se_dev->dev_attrib.block_size;
> -
> ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
> &cmd->t_bidi_data_nents,
> - caw_length, zero_flag, false);
> + cmd->buffer_length, zero_flag, false);
> if (ret < 0)
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 49cd03c2d943..0660585cb03d 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -455,7 +455,9 @@ struct se_cmd {
> enum transport_state_table t_state;
> /* See se_cmd_flags_table */
> u32 se_cmd_flags;
> - /* Total size in bytes associated with command */
> + /* Length of Data-Out or Data-In buffer */
> + u32 buffer_length;
> + /* Number of bytes affected on storage medium */
> u32 data_length;
> u32 residual_count;
> u64 orig_fe_lun;