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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2015-04-26 18:44 GMT+09:00 Sagi Grimberg <sagig@xxxxxxxxxxxxxxxxxx>:
>>> @@ -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.

>>>       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'd say this needs to be:
>>
>> if (cmd->prot_op != TARGET_PROT_NORMAL &&
>>      !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux