| Message ID | 20191030090847.25650-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 9237f04e12cc385334043cd7cf84b74dcbda0256 |
| Headers | show |
| Series | [v2] scsi: Fix scsi_get/set_resid() interface | expand |
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)); else if (unlikely(rsp->flags & SRP_RSP_FLAG_DIOVER)) scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_in_res_cnt)); 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.
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. >
On 10/30/19 9:21 AM, Damien Le Moal wrote: > 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... Hi Damien, What I'm wondering about is how the SCSI core should support residual overflow. Should a new member be introduced in struct scsi_request? Should resid_len be changed from unsigned int to int or should we perhaps follow yet another approach? Thanks, Bart.
On 10/30/19 6:01 PM, Bart Van Assche wrote: > On 10/30/19 9:21 AM, Damien Le Moal wrote: >> 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... > > Hi Damien, > > What I'm wondering about is how the SCSI core should support residual > overflow. Should a new member be introduced in struct scsi_request? > Should resid_len be changed from unsigned int to int or should we > perhaps follow yet another approach? > Please introduce a new member to hold any overflow value. And document where it is needed. Cheers, Hannes
Bart, On 2019/10/31 17:39, Hannes Reinecke wrote: > On 10/30/19 6:01 PM, Bart Van Assche wrote: >> On 10/30/19 9:21 AM, Damien Le Moal wrote: >>> 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... >> >> Hi Damien, >> >> What I'm wondering about is how the SCSI core should support residual >> overflow. Should a new member be introduced in struct scsi_request? >> Should resid_len be changed from unsigned int to int or should we >> perhaps follow yet another approach? >> > Please introduce a new member to hold any overflow value. > And document where it is needed. Yes, I also think this is the best approach. The current resid_len member of struct scsi_request can be kept as is, encoding actual underflow of a command (less bytes transferred than asked for). The new field would only be used in the case of "overflow", which are not actual buffer overflows as Hannes pointed out (otherwise, we would get memory corruptions, iommu screaming etc). The SG driver can make use of this field to keep the io header resid as an int, with negative values indicating overflows and positive values underflows. Best regards.
Damien, > The SG driver can make use of this field to keep the io header resid > as an int, with negative values indicating overflows and positive > values underflows. I am all for synthesizing what SG returns to userland. That is also the case in the context of Hannes' SCSI result revamp. I would much prefer to have well-defined and consistent internal kernel status fields and then transmogrify those into something compatible with what userland applications might expect. As opposed to perpetuating the train wreck that is the current scsi_cmnd result.
On 2019/11/05 14:19, Martin K. Petersen wrote: > > Damien, > >> The SG driver can make use of this field to keep the io header resid >> as an int, with negative values indicating overflows and positive >> values underflows. > > I am all for synthesizing what SG returns to userland. > > That is also the case in the context of Hannes' SCSI result revamp. I > would much prefer to have well-defined and consistent internal kernel > status fields and then transmogrify those into something compatible with > what userland applications might expect. As opposed to perpetuating the > train wreck that is the current scsi_cmnd result. OK. But just to clarify, do you mean changing struct scsi_request resid_len field to an int and considering positive values of it as underflow and negative values as overflow ? Or keeping resid_len as an unsigned int and adding a flag specifying if the value means underflow or overflow ? I am rely surprised that my simple patch resulted in such a big discussion. But if that leads to code improvements, then let's drop it and work on a new series !
Damien, > Or keeping resid_len as an unsigned int and adding a flag specifying > if the value means underflow or overflow ? It's been broken for so long I'd rather make the overflow case an opt-in. So a separate flag, please.
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. Since there is agreement that residual overflow should use another field than scsi_request.resid_len: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Damien, > 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. Applied to 5.5/scsi-queue, thanks!
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index d323523f5f9d..4daf2637c011 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1025,7 +1025,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr, static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, int arr_len, unsigned int off_dst) { - int act_len, n; + unsigned int act_len, n; struct scsi_data_buffer *sdb = &scp->sdb; off_t skip = off_dst; @@ -1039,7 +1039,7 @@ static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, pr_debug("%s: off_dst=%u, scsi_bufflen=%u, act_len=%u, resid=%d\n", __func__, off_dst, scsi_bufflen(scp), act_len, scsi_get_resid(scp)); - n = (int)scsi_bufflen(scp) - ((int)off_dst + act_len); + n = scsi_bufflen(scp) - (off_dst + act_len); scsi_set_resid(scp, min(scsi_get_resid(scp), n)); return 0; } diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c index 8b1b73065421..98c1aa594e6c 100644 --- a/drivers/usb/storage/ene_ub6250.c +++ b/drivers/usb/storage/ene_ub6250.c @@ -561,7 +561,7 @@ static int ene_send_scsi_cmd(struct us_data *us, u8 fDir, void *buf, int use_sg) residue = min(residue, transfer_length); if (us->srb != NULL) scsi_set_resid(us->srb, max(scsi_get_resid(us->srb), - (int)residue)); + residue)); } if (bcs->Status != US_BULK_STAT_OK) diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c index 96cb0409dd89..238a8088e17f 100644 --- a/drivers/usb/storage/transport.c +++ b/drivers/usb/storage/transport.c @@ -1284,8 +1284,7 @@ int usb_stor_Bulk_transport(struct scsi_cmnd *srb, struct us_data *us) } else { residue = min(residue, transfer_length); - scsi_set_resid(srb, max(scsi_get_resid(srb), - (int) residue)); + scsi_set_resid(srb, max(scsi_get_resid(srb), residue)); } } diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 91bd749a02f7..3772ae5d40cd 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -190,12 +190,12 @@ static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd) return cmd->sdb.length; } -static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid) +static inline void scsi_set_resid(struct scsi_cmnd *cmd, unsigned int resid) { cmd->req.resid_len = resid; } -static inline int scsi_get_resid(struct scsi_cmnd *cmd) +static inline unsigned int scsi_get_resid(struct scsi_cmnd *cmd) { return cmd->req.resid_len; }
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. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- Changes from v1: * Fix compilation error in the USB ENE card reader driver drivers/scsi/scsi_debug.c | 4 ++-- drivers/usb/storage/ene_ub6250.c | 2 +- drivers/usb/storage/transport.c | 3 +-- include/scsi/scsi_cmnd.h | 4 ++-- 4 files changed, 6 insertions(+), 7 deletions(-)