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

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

[Damien Le Moal]

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(-)

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;
 }
-- 
2.21.0

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

[Bart Van Assche]

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.

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

[Damien Le Moal]

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

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

[Bart Van Assche]

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.

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

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

[Damien Le Moal]

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 Le Moal
Western Digital Research

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

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Damien Le Moal]

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 Le Moal
Western Digital Research

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

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Bart Van Assche]

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>

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

[Martin K. Petersen]

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!

-- 
Martin K. Petersen	Oracle Linux Engineering