Re: [PATCH v3 1/5] target: ensure se_cmd->t_prot_sg is allocated when required

[Sagi Grimberg <sagig@dev.mellanox.co.il>]

On 4/26/2015 12:26 PM, Sagi Grimberg wrote:
> On 4/25/2015 5:33 PM, Akinobu Mita wrote:
>> Even if the device backend is initialized with protection info is
>> enabled, some requests don't have the protection info attached for
>> WRITE SAME command issued by block device helpers, WRITE command with
>> WRPROTECT=0 by SG_IO ioctl, etc.
>>
>> So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL
>> for these requests and performing WRITE_INSERT of PI using software
>> emulation by sbc_dif_generate() causes kernel crash.
>>
>> To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for
>> se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs
>> to be allocated or use pre-allocated protection information by scsi
>> mid-layer.
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
>> Cc: Sagi Grimberg <sagig@mellanox.com>
>> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
>> Cc: target-devel@vger.kernel.org
>> Cc: linux-scsi@vger.kernel.org
>> ---
>> * No change from v2
>>
>>   drivers/target/target_core_transport.c | 30
>> ++++++++++++++++++------------
>>   include/target/target_core_base.h      |  1 +
>>   2 files changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c
>> b/drivers/target/target_core_transport.c
>> index 7a9e7e2..fe52883 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd
>> *se_cmd, struct se_session *se_sess
>>       if (sgl_prot_count) {
>>           se_cmd->t_prot_sg = sgl_prot;
>>           se_cmd->t_prot_nents = sgl_prot_count;
>> +        se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC;
>>       }
>>
>>       /*
>> @@ -2181,6 +2182,12 @@ static inline void
>> transport_reset_sgl_orig(struct se_cmd *cmd)
>>
>>   static inline void transport_free_pages(struct se_cmd *cmd)
>>   {
>> +    if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
>> +        transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
>> +        cmd->t_prot_sg = NULL;
>> +        cmd->t_prot_nents = 0;
>> +    }
>> +
>
> Hi Akinobu,
>
> Any reason why this changed it's location to the start of the function?
>
>>       if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) {
>>           /*
>>            * Release special case READ buffer payload required for
>> @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct
>> se_cmd *cmd)
>>       transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents);
>>       cmd->t_bidi_data_sg = NULL;
>>       cmd->t_bidi_data_nents = 0;
>> -
>> -    transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
>> -    cmd->t_prot_sg = NULL;
>> -    cmd->t_prot_nents = 0;
>>   }
>>
>>   /**
>> @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd)
>>       int ret = 0;
>>       bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB);
>>
>> +    if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
>> +        if (cmd->prot_op != TARGET_PROT_NORMAL) {
>
> This seems wrong,
>
> What will happen for transports that will actually to allocate
> protection SGLs? The allocation is unreachable since
> SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set...

Umm, actually this is reachable... But I still think the condition
should be the other way around (saving a condition in some common
cases).

>
> I'd say this needs to be:
>
> if (cmd->prot_op != TARGET_PROT_NORMAL &&
>      !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
>
>> +            ret = target_alloc_sgl(&cmd->t_prot_sg,
>> +                           &cmd->t_prot_nents,
>> +                           cmd->prot_length, true);
>> +            if (ret < 0)
>> +                return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>> +        }
>> +
>> +    }
>> +
>>       /*
>>        * Determine is the TCM fabric module has already allocated
>> physical
>>        * memory, and is directly calling
>> transport_generic_map_mem_to_cmd()
>> @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd)
>>                   return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>           }
>>
>> -        if (cmd->prot_op != TARGET_PROT_NORMAL) {
>> -            ret = target_alloc_sgl(&cmd->t_prot_sg,
>> -                           &cmd->t_prot_nents,
>> -                           cmd->prot_length, true);
>> -            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);
>>           if (ret < 0)
>> diff --git a/include/target/target_core_base.h
>> b/include/target/target_core_base.h
>> index 480e9f8..13efcdd 100644
>> --- a/include/target/target_core_base.h
>> +++ b/include/target/target_core_base.h
>> @@ -167,6 +167,7 @@ enum se_cmd_flags_table {
>>       SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
>>       SCF_COMPARE_AND_WRITE        = 0x00080000,
>>       SCF_COMPARE_AND_WRITE_POST    = 0x00100000,
>> +    SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
>>   };
>>
>>   /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
>>
>