| Message ID | 20191028105732.29913-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: Fix scsi_get/set_resid() interface | expand |
On 10/28/19 3:57 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. > [ ... ] > -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; > } From the iSCSI RFC: SCSI-Presented Data Transfer Length (SPDTL) is the term this document uses (see Section 1.1 for definition) to represent the aggregate data length that the target SCSI layer attempts to transfer using the local iSCSI layer for a task. Expected Data Transfer Length (EDTL) is the iSCSI term that represents the length of data that the iSCSI layer expects to transfer for a task. EDTL is specified in the SCSI Command PDU. When SPDTL = EDTL for a task, the target iSCSI layer completes the task with no residuals. Whenever SPDTL differs from EDTL for a task, that task is said to have a residual. If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST be set to the numerical value of (SPDTL - EDTL). If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST be set to the numerical value of (EDTL - SPDTL). Note that the Overflow and Underflow scenarios are independent of Data-In and Data-Out. Either scenario is logically possible in either direction of data transfer. If the residual is changed from signed into unsigned, how is a SCSI LLD expected to report the difference between residual overflow and residual underflow to the SCSI core? Thanks, Bart.
Bart, On 2019/10/28 21:38, Bart Van Assche wrote: > On 10/28/19 3:57 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. >> [ ... ] >> -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; >> } > > From the iSCSI RFC: > > SCSI-Presented Data Transfer Length (SPDTL) is the term this document > uses (see Section 1.1 for definition) to represent the aggregate data > length that the target SCSI layer attempts to transfer using the > local iSCSI layer for a task. Expected Data Transfer Length (EDTL) > is the iSCSI term that represents the length of data that the iSCSI > layer expects to transfer for a task. EDTL is specified in the SCSI > Command PDU. > > When SPDTL = EDTL for a task, the target iSCSI layer completes the > task with no residuals. Whenever SPDTL differs from EDTL for a task, > that task is said to have a residual. > > If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > be set to the numerical value of (SPDTL - EDTL). > > If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > be set to the numerical value of (EDTL - SPDTL). > > Note that the Overflow and Underflow scenarios are independent of > Data-In and Data-Out. Either scenario is logically possible in > either direction of data transfer. > > If the residual is changed from signed into unsigned, how is a SCSI LLD > expected to report the difference between residual overflow and residual > underflow to the SCSI core? As mentioned in the commit message, cmd->req.resid_len where the resid is stored is an unsigned int. And it has been an unsigned int forever as far as I know. So I think changing the interface to unsigned int makes sense in that context. Also, unless I am reading this wrong, the above definition you quote always lead to resid >= 0, so I do not see what problem you are worried about. Could you elaborate your concerns please ? > > Thanks, > > Bart. >
On 10/29/19 1:17 AM, Damien Le Moal wrote: > So I think changing the interface to unsigned int makes sense in that > context. Also, unless I am reading this wrong, the above definition you > quote always lead to resid >= 0, so I do not see what problem you are > worried about. Could you elaborate your concerns please ? Hi Damien, SCSI LLDs handle residual overflow in at least two different ways. I think the iSCSI initiator handles this by reporting the DID_BAD_TARGET error code. In the tcm_loop driver I found the following, which I think is incorrect: set_host_byte(sc, DID_OK); if ((se_cmd->se_cmd_flags & SCF_OVERFLOW_BIT) || (se_cmd->se_cmd_flags & SCF_UNDERFLOW_BIT)) scsi_set_resid(sc, se_cmd->residual_count); Seeing this made me wonder what the best approach is for a SCSI LLD to report a residual overflow? Thanks, Bart.
On 2019-10-29 4:17 a.m., Damien Le Moal wrote: > Bart, > > On 2019/10/28 21:38, Bart Van Assche wrote: >> On 10/28/19 3:57 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. >>> [ ... ] >>> -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; >>> } >> >> From the iSCSI RFC: >> >> SCSI-Presented Data Transfer Length (SPDTL) is the term this document >> uses (see Section 1.1 for definition) to represent the aggregate data >> length that the target SCSI layer attempts to transfer using the >> local iSCSI layer for a task. Expected Data Transfer Length (EDTL) >> is the iSCSI term that represents the length of data that the iSCSI >> layer expects to transfer for a task. EDTL is specified in the SCSI >> Command PDU. >> >> When SPDTL = EDTL for a task, the target iSCSI layer completes the >> task with no residuals. Whenever SPDTL differs from EDTL for a task, >> that task is said to have a residual. >> >> If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the >> SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST >> be set to the numerical value of (SPDTL - EDTL). >> >> If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the >> SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST >> be set to the numerical value of (EDTL - SPDTL). >> >> Note that the Overflow and Underflow scenarios are independent of >> Data-In and Data-Out. Either scenario is logically possible in >> either direction of data transfer. >> >> If the residual is changed from signed into unsigned, how is a SCSI LLD >> expected to report the difference between residual overflow and residual >> underflow to the SCSI core? > As mentioned in the commit message, cmd->req.resid_len where the resid > is stored is an unsigned int. And it has been an unsigned int forever as > far as I know. If my memory serves, I put it in over 20 years ago, about the same time as the sg v2 interface (struct sg_io_hdr) was implemented. And when I put it in it was a (signed) int and this is why: struct ccb_accept_targ3 { ..... CAM_I32 cam_resid; /* Transfer residual length: 2's comp */ ..... }; [from cam3r03.pdf] So it coped with both underflow and the less likely case of overflow. Notice in: struct sg_io_hdr { ..... int resid; /* [o] dxfer_len - actual_transferred */ ... }; At the time the SCSI mid-level and the block layer didn't bother about resid, only some LLDs which is where the sg driver picked it up from. So some ignoramus changed it to unsigned since then, probably before git strangled the kernel source tree. > So I think changing the interface to unsigned int makes sense in that > context. Also, unless I am reading this wrong, the above definition you > quote always lead to resid >= 0, so I do not see what problem you are > worried about. Could you elaborate your concerns please ? Linux's internal representation is wrong and should be fixed to match its exposed interface (struct sg_io_hdr) which predates the breakage. Doug Gilbert
On 2019-10-29 10:59, Douglas Gilbert wrote: > Linux's internal representation is wrong and should be fixed to match > its exposed interface (struct sg_io_hdr) which predates the breakage. If resid_len is changed from unsigned int to int all SCSI core and SCSI LLD code that uses resid_len will have to be reviewed to see whether it handles residual overflow correctly. Bart.
On 10/28/19 9:38 PM, Bart Van Assche wrote: > On 10/28/19 3:57 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. >> [ ... ] >> -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; >> } > > From the iSCSI RFC: > > SCSI-Presented Data Transfer Length (SPDTL) is the term this document > uses (see Section 1.1 for definition) to represent the aggregate data > length that the target SCSI layer attempts to transfer using the > local iSCSI layer for a task. Expected Data Transfer Length (EDTL) > is the iSCSI term that represents the length of data that the iSCSI > layer expects to transfer for a task. EDTL is specified in the SCSI > Command PDU. > > When SPDTL = EDTL for a task, the target iSCSI layer completes the > task with no residuals. Whenever SPDTL differs from EDTL for a task, > that task is said to have a residual. > > If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > be set to the numerical value of (SPDTL - EDTL). > > If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the > SCSI Response PDU as specified in [RFC3720]. The Residual Count MUST > be set to the numerical value of (EDTL - SPDTL). > > Note that the Overflow and Underflow scenarios are independent of > Data-In and Data-Out. Either scenario is logically possible in > either direction of data transfer. > > If the residual is changed from signed into unsigned, how is a SCSI LLD > expected to report the difference between residual overflow and residual > underflow to the SCSI core? > You don't have to. To quote RFC 3720 page 122: bit 5 - (O) set for Residual Overflow. In this case, the Residual Count indicates the number of bytes that were not transferred because the initiator's Expected Data Transfer Length was not sufficient. For a bidirectional operation, the Residual Count contains the residual for the write operation. IE the 'overflow' setting in the iSCSI command response is an indicator that there _would_ be more data if the command request _would_ have specified a larger buffer. But as it didn't, the entire buffer was filled, and the overflow counter is set. Which, of course, is then ignored by the linux SCSI stack as the request got all data, and the residual is set to zero. Then it's left to the caller to re-send with a larger buffer if required. But it's nothing the SCSI stack can nor should be attempting on its own. As such I think the patch is correct. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 10/30/19 1:30 AM, Hannes Reinecke wrote: > On 10/28/19 9:38 PM, Bart Van Assche wrote: >> If the residual is changed from signed into unsigned, how is a SCSI >> LLD expected to report the difference between residual overflow and >> residual underflow to the SCSI core? > > You don't have to. To quote RFC 3720 page 122: > > bit 5 - (O) set for Residual Overflow. In this case, the Residual > Count indicates the number of bytes that were not transferred > because the initiator's Expected Data Transfer Length was not > sufficient. For a bidirectional operation, the Residual Count > contains the residual for the write operation. > > IE the 'overflow' setting in the iSCSI command response is an indicator > that there _would_ be more data if the command request _would_ have > specified a larger buffer. > But as it didn't, the entire buffer was filled, and the overflow counter > is set. > Which, of course, is then ignored by the linux SCSI stack as the request > got all data, and the residual is set to zero. > Then it's left to the caller to re-send with a larger buffer if > required. But it's nothing the SCSI stack can nor should be attempting > on its own. Hi Hannes, I do not agree that reporting a residual overflow by calling scsi_set_resid(..., 0) is acceptable. For reads a residual overflow means that the length specified in the CDB (scsi_bufflen()) exceeds the data buffer size (length of scsi_sglist()). I think it's dangerous to report to the block layer that such requests completed successfully and with residual zero. Bart.
On 10/30/19 4:12 PM, Bart Van Assche wrote: > On 10/30/19 1:30 AM, Hannes Reinecke wrote: >> On 10/28/19 9:38 PM, Bart Van Assche wrote: >>> If the residual is changed from signed into unsigned, how is a SCSI >>> LLD expected to report the difference between residual overflow and >>> residual underflow to the SCSI core? >> >> You don't have to. To quote RFC 3720 page 122: >> >> bit 5 - (O) set for Residual Overflow. In this case, the Residual >> Count indicates the number of bytes that were not transferred >> because the initiator's Expected Data Transfer Length was not >> sufficient. For a bidirectional operation, the Residual Count >> contains the residual for the write operation. >> >> IE the 'overflow' setting in the iSCSI command response is an >> indicator that there _would_ be more data if the command request >> _would_ have specified a larger buffer. >> But as it didn't, the entire buffer was filled, and the overflow >> counter is set. >> Which, of course, is then ignored by the linux SCSI stack as the >> request got all data, and the residual is set to zero. >> Then it's left to the caller to re-send with a larger buffer if >> required. But it's nothing the SCSI stack can nor should be attempting >> on its own. > > Hi Hannes, > > I do not agree that reporting a residual overflow by calling > scsi_set_resid(..., 0) is acceptable. For reads a residual overflow > means that the length specified in the CDB (scsi_bufflen()) exceeds the > data buffer size (length of scsi_sglist()). I think it's dangerous to > report to the block layer that such requests completed successfully and > with residual zero. > But that is an error on submission, and should be aborted before it even got send to the drive. However, this does not relate to the residual, which is handled after the command completes (and which sparked this entire thread ...). Cheers, Hannes
On 10/30/19 8:18 AM, Hannes Reinecke wrote: > On 10/30/19 4:12 PM, Bart Van Assche wrote: >> I do not agree that reporting a residual overflow by calling >> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow >> means that the length specified in the CDB (scsi_bufflen()) exceeds >> the data buffer size (length of scsi_sglist()). I think it's dangerous >> to report to the block layer that such requests completed successfully >> and with residual zero. >> > But that is an error on submission, and should be aborted before it even > got send to the drive. If such a bug ever gets introduced in the SCSI core, I think that SCSI target code should detect and report it. If the SCSI core receives a response with a residual overflow it can then take appropriate action, e.g. call WARN_ON_ONCE(). Users of sg_raw can trigger the residual overflow case easily. > However, this does not relate to the residual, which is handled after > the command completes (and which sparked this entire thread ...). I'm still waiting for an answer to my question of how SCSI LLDs are expected to report a residual overflow to the SCSI core. Thanks, Bart.
On 2019-10-30 11:18 a.m., Hannes Reinecke wrote: > On 10/30/19 4:12 PM, Bart Van Assche wrote: >> On 10/30/19 1:30 AM, Hannes Reinecke wrote: >>> On 10/28/19 9:38 PM, Bart Van Assche wrote: >>>> If the residual is changed from signed into unsigned, how is a SCSI LLD >>>> expected to report the difference between residual overflow and residual >>>> underflow to the SCSI core? >>> >>> You don't have to. To quote RFC 3720 page 122: >>> >>> bit 5 - (O) set for Residual Overflow. In this case, the Residual >>> Count indicates the number of bytes that were not transferred >>> because the initiator's Expected Data Transfer Length was not >>> sufficient. For a bidirectional operation, the Residual Count >>> contains the residual for the write operation. >>> >>> IE the 'overflow' setting in the iSCSI command response is an indicator that >>> there _would_ be more data if the command request _would_ have specified a >>> larger buffer. >>> But as it didn't, the entire buffer was filled, and the overflow counter is set. >>> Which, of course, is then ignored by the linux SCSI stack as the request got >>> all data, and the residual is set to zero. >>> Then it's left to the caller to re-send with a larger buffer if required. But >>> it's nothing the SCSI stack can nor should be attempting on its own. >> >> Hi Hannes, >> >> I do not agree that reporting a residual overflow by calling >> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow means that >> the length specified in the CDB (scsi_bufflen()) exceeds the data buffer size >> (length of scsi_sglist()). I think it's dangerous to report to the block layer >> that such requests completed successfully and with residual zero. >> > But that is an error on submission, and should be aborted before it even got > send to the drive. > > However, this does not relate to the residual, which is handled after the > command completes (and which sparked this entire thread ...). Seen from a pass-through perspective, the resid is just about the near-end data transfer between the HBA and pass-through, or as cam3r03 says: − cam_resid; The data residual length member contains the difference in twos complement form of the number of data bytes transferred by the HA for the SCSI command compared with the number of bytes requested by the CCB cam_dxfer_len member. This is calculated by the total number of bytes requested to be transferred by the CCB minus the actual number of bytes transferred by the HA. [where "HA" is HBA (or the initiator)] That makes overflow (negative resid) a bit interesting as it is only reasonable that the pass-though user allocated a buffer big enough to receive dxfer_len bytes. One would hope that in the READ case of overflow, the HBA would have sent the residual bytes to the bit bucket (i.e. /dev/null) rather than overfill the data buffer provided by the pass-through. Handling discrepancies between page length (e.g. of VPD, LOG and MODE pages) and the ALLOCATION LENGTH field are defined in the SCSI standards. That leaves the difficult cases: when there is a discrepancy between what the SCSI command (and the storage device) implied as a data length and the dxfer_len allocated at the near-end. In some, but not all, cases that is detectable before the command is issued. Overflow can happen, for example if the RDPROTECT field in a READ(10) is accidentally set (e.g. because it uses bits previously used for a SPI LUN) and the storage device has protection information. That will result in an extra 8 bytes per logical block being returned. Doug Gilbert
On 10/30/19 5:18 PM, Douglas Gilbert wrote: > On 2019-10-30 11:18 a.m., Hannes Reinecke wrote: >> On 10/30/19 4:12 PM, Bart Van Assche wrote: >>> On 10/30/19 1:30 AM, Hannes Reinecke wrote: >>>> On 10/28/19 9:38 PM, Bart Van Assche wrote: >>>>> If the residual is changed from signed into unsigned, how is a SCSI >>>>> LLD expected to report the difference between residual overflow and >>>>> residual underflow to the SCSI core? >>>> >>>> You don't have to. To quote RFC 3720 page 122: >>>> >>>> bit 5 - (O) set for Residual Overflow. In this case, the >>>> Residual >>>> Count indicates the number of bytes that were not transferred >>>> because the initiator's Expected Data Transfer Length was not >>>> sufficient. For a bidirectional operation, the Residual Count >>>> contains the residual for the write operation. >>>> >>>> IE the 'overflow' setting in the iSCSI command response is an >>>> indicator that there _would_ be more data if the command request >>>> _would_ have specified a larger buffer. >>>> But as it didn't, the entire buffer was filled, and the overflow >>>> counter is set. >>>> Which, of course, is then ignored by the linux SCSI stack as the >>>> request got all data, and the residual is set to zero. >>>> Then it's left to the caller to re-send with a larger buffer if >>>> required. But it's nothing the SCSI stack can nor should be >>>> attempting on its own. >>> >>> Hi Hannes, >>> >>> I do not agree that reporting a residual overflow by calling >>> scsi_set_resid(..., 0) is acceptable. For reads a residual overflow >>> means that the length specified in the CDB (scsi_bufflen()) exceeds >>> the data buffer size (length of scsi_sglist()). I think it's >>> dangerous to report to the block layer that such requests completed >>> successfully and with residual zero. >>> >> But that is an error on submission, and should be aborted before it >> even got send to the drive. >> >> However, this does not relate to the residual, which is handled after >> the command completes (and which sparked this entire thread ...). > > Seen from a pass-through perspective, the resid is just about the near-end > data transfer between the HBA and pass-through, or as cam3r03 says: > > − cam_resid; > The data residual length member contains the difference in twos > complement > form of the number of data bytes transferred by the HA for the SCSI > command compared with the number of bytes requested by the CCB > cam_dxfer_len member. This is calculated by the total number of bytes > requested to be transferred by the CCB minus the actual number of > bytes > transferred by the HA. > > [where "HA" is HBA (or the initiator)] > > That makes overflow (negative resid) a bit interesting as it is only > reasonable that the pass-though user allocated a buffer big enough to > receive dxfer_len bytes. One would hope that in the READ case of overflow, > the HBA would have sent the residual bytes to the bit bucket (i.e. > /dev/null) rather than overfill the data buffer provided by the > pass-through. > Yes. But my point here is that any residual values are handled (and defined) at the protocol level. Any SCSI midlayer protocol like SPC or, indeed, SAM have no concept of residuals. THe SCSI midlayer chose to add a residual value to document the number of bytes _not_ being handled from hardware. > Handling discrepancies between page length (e.g. of VPD, LOG and MODE > pages) and the ALLOCATION LENGTH field are defined in the SCSI standards. > > That leaves the difficult cases: when there is a discrepancy between > what the > SCSI command (and the storage device) implied as a data length and the > dxfer_len allocated at the near-end. In some, but not all, cases that is > detectable before the command is issued. > As mentioned, this needs to be handled in the submission path, and the command should be aborted before being sent to the drive. > Overflow can happen, for example if the RDPROTECT field in a READ(10) is > accidentally set (e.g. because it uses bits previously used for a SPI > LUN) and the storage device has protection information. That will result > in an extra 8 bytes per logical block being returned. > See above. We should intercept such a command, and simply abort it with an appropriate error. That goes for internal commands, of course; I know it doesn't work for things like sg_raw, but one could argue that this was precisely the point of that command ... Cheers, Hannes
On 10/30/19 4:12 PM, Bart Van Assche wrote: > On 10/30/19 1:30 AM, Hannes Reinecke wrote: >> On 10/28/19 9:38 PM, Bart Van Assche wrote: >>> If the residual is changed from signed into unsigned, how is a SCSI >>> LLD expected to report the difference between residual overflow and >>> residual underflow to the SCSI core? >> >> You don't have to. To quote RFC 3720 page 122: >> >> bit 5 - (O) set for Residual Overflow. In this case, the Residual >> Count indicates the number of bytes that were not transferred >> because the initiator's Expected Data Transfer Length was not >> sufficient. For a bidirectional operation, the Residual Count >> contains the residual for the write operation. >> >> IE the 'overflow' setting in the iSCSI command response is an >> indicator that there _would_ be more data if the command request >> _would_ have specified a larger buffer. >> But as it didn't, the entire buffer was filled, and the overflow >> counter is set. >> Which, of course, is then ignored by the linux SCSI stack as the >> request got all data, and the residual is set to zero. >> Then it's left to the caller to re-send with a larger buffer if >> required. But it's nothing the SCSI stack can nor should be attempting >> on its own. > > Hi Hannes, > > I do not agree that reporting a residual overflow by calling > scsi_set_resid(..., 0) is acceptable. For reads a residual overflow > means that the length specified in the CDB (scsi_bufflen()) exceeds the > data buffer size (length of scsi_sglist()). I think it's dangerous to > report to the block layer that such requests completed successfully and > with residual zero. > Dangerous how? fcp3: For read operations and write operations, if the FCP_RESID_OVER bit is set to one, the FCP_RESID field contains the excess of the number of bytes required by the SCSI command to be transferred over the number of bytes specified by the FCP_DL field. SAS doesn't even have the notion of residuals srp04: DOOVER , when set to one, indicates that the DATA-OUT RESIDUAL COUNT field is valid and contains the count of data bytes that could not be transferred from the data-out buffer because the length of the data-out buffer was not sufficient. The application client should examine the DATA-OUT RESIDUAL COUNT field in the context of the command to determine whether or not an error condition occurred. iSCSI we've already covered. In all cases, the overflow value is an _indicator_ that additional data is available, but was not transferred due to the lack of space. So we will not have any buffer overflow as data is never transferred. And in most cases an overflow is actually desired; it is a pretty common use pattern to send a SCSI command with a small enough buffer to return the length of available data, and then send the actual command knowing how large the buffer needs to be. See for example scsi_report_lun_scan() or the VPD handling code. So again, I don't think it's something we need to worry about. Cheers, Hannes
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/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 and usb transport code. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/scsi/scsi_debug.c | 4 ++-- drivers/usb/storage/transport.c | 3 +-- include/scsi/scsi_cmnd.h | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-)