Re: [PATCH v2] scsi: Fix scsi_get/set_resid() interface

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2019/10/30 16:15, Bart Van Assche wrote:
> On 10/30/19 2:08 AM, Damien Le Moal wrote:
>> struct scsi_cmnd cmd->req.resid_len which is returned and set
>> respectively by the helper functions scsi_get_resid() and
>> scsi_set_resid() is an unsigned int. Reflect this fact in the interface
>> of these helper functions.
>>
>> Also fix compilation errors due to min() and max() type mismatch
>> introduced by this change in scsi debug code, usb transport code and in
>> the USB ENE card reader driver.
>   Please answer my question about how a SCSI LLD should report residual 
> overflows. I think this patch is incompatible with the approach used by 
> the SRP initiator driver:
> 
> if (unlikely(rsp->flags & SRP_RSP_FLAG_DIUNDER))
> 	scsi_set_resid(scmnd, be32_to_cpu(rsp->data_in_res_cnt));

be32_to_cpu(rsp->data_in_res_cnt) is always >= 0 so no problems.

> else if (unlikely(rsp->flags & SRP_RSP_FLAG_DIOVER))
> 	scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_in_res_cnt));

-be32_to_cpu(rsp->data_in_res_cnt) is always <= 0. But it does *not*
matter if my patch is applied or not, this negative value gets stored
into scmd->req.resid_len which is an *unsigned int*.
How does that work ?

My patch changes the function resid argument type and that function is
inline, so in practice, there are 0 changes, none whatsoever, isn't it ?

The problem you are raising here may exist, and how the scsi core will
handle a negative value cast to an unsigned int, likely resulting in a
value far larger than the command buffer size, is certainly a serious
concern. But it is unrelated to what my patch does.

If you feel strongly about it, I have absolutely no problem with
dropping the patch. I just would like that it be dropped for the right
reasons...

> else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOUNDER))
> 	scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt));
> else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOOVER))
> 	scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_out_res_cnt));
> 
> Thanks,
> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research