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

Sagi Grimberg
2015-04-27T15:08:41Z

On 4/27/2015 3:57 PM, Akinobu Mita wrote:
> 2015-04-26 18:44 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
>>>> @@ -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?
>
> Because when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is set, it will not
> reach the tail of the function. So when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC
> is cleared and SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is set,
> se_cmd->t_prot_sg leaks.

I see. That's fine...

>
>>>> 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).
>
> Do you mean you prefer below?
>
> if (cmd->prot_op != TARGET_PROT_NORMAL &&
> !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
> ...
>

I think it will be better.