[PATCH] scsi_debug: rework resp_report_luns

[PATCH] scsi_debug: rework resp_report_luns

[Tomas Winkler]

1. Fix the error check: the alloc length should be > 16
and not > 4
2. Remove duplicated boundary checks which simplify
the fill-in loop
3. Use more of scsi generic API

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/scsi/scsi_debug.c | 119 +++++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 94c18e300135..4dc8caad6cb2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3320,63 +3320,96 @@ resp_get_lba_status(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return fill_from_dev_buffer(scp, arr, SDEBUG_GET_LBA_STATUS_LEN);
 }
 
-#define SDEBUG_RLUN_ARR_SZ 256
+#define SDEBUG_MAX_RLUNS 31
+#define SDEBUG_RLUN_ARR_SZ ((SDEBUG_MAX_RLUNS * 8) + 8)
 
-static int resp_report_luns(struct scsi_cmnd * scp,
-			    struct sdebug_dev_info * devip)
+static int resp_report_luns(struct scsi_cmnd *scp,
+			    struct sdebug_dev_info *devip)
 {
+	unsigned char *cmd = scp->cmnd;
 	unsigned int alloc_len;
-	int lun_cnt, i, upper, num, n, want_wlun, shortish;
+	unsigned char select_report;
+
 	u64 lun;
-	unsigned char *cmd = scp->cmnd;
-	int select_report = (int)cmd[2];
 	struct scsi_lun *one_lun;
-	unsigned char arr[SDEBUG_RLUN_ARR_SZ];
-	unsigned char * max_addr;
+	u8 arr[SDEBUG_RLUN_ARR_SZ];
+	unsigned int lun_cnt;          /* LUN count */
+	unsigned int wlun_cnt;         /* W_LUN count */
+	unsigned int rlun_cnt;         /* reported LUN count */
+	unsigned int rlen;             /* reported luns in bytes */
+	unsigned int n;
+	int i;
 
 	clear_luns_changed_on_target(devip);
-	alloc_len = cmd[9] + (cmd[8] << 8) + (cmd[7] << 16) + (cmd[6] << 24);
-	shortish = (alloc_len < 4);
-	if (shortish || (select_report > 2)) {
-		mk_sense_invalid_fld(scp, SDEB_IN_CDB, shortish ? 6 : 2, -1);
+
+	select_report = cmd[2];
+	alloc_len = get_unaligned_be32(cmd + 6);
+
+	if (alloc_len < 16) {
+		pr_err("alloc len too small %d\n", alloc_len);
+		mk_sense_invalid_fld(scp, SDEB_IN_CDB, 6, -1);
 		return check_condition_result;
 	}
-	/* can produce response with up to 16k luns (lun 0 to lun 16383) */
-	memset(arr, 0, SDEBUG_RLUN_ARR_SZ);
+
+	if (select_report > 0x02) {
+		pr_err("select report invalid %d\n", select_report);
+		mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
+		return check_condition_result;
+	}
+
 	lun_cnt = scsi_debug_max_luns;
-	if (1 == select_report)
+	wlun_cnt = 0;
+
+	/* report only w_lun */
+	if (select_report == 0x01)
 		lun_cnt = 0;
-	else if (scsi_debug_no_lun_0 && (lun_cnt > 0))
+
+	if (scsi_debug_no_lun_0 && (lun_cnt > 0))
 		--lun_cnt;
-	want_wlun = (select_report > 0) ? 1 : 0;
-	num = lun_cnt + want_wlun;
-	arr[2] = ((sizeof(struct scsi_lun) * num) >> 8) & 0xff;
-	arr[3] = (sizeof(struct scsi_lun) * num) & 0xff;
-	n = min((int)((SDEBUG_RLUN_ARR_SZ - 8) /
-			    sizeof(struct scsi_lun)), num);
-	if (n < num) {
-		want_wlun = 0;
+
+	/* report w_lun */
+	if (select_report == 0x01 || select_report == 0x02)
+		wlun_cnt = 1;
+
+	rlun_cnt = lun_cnt + wlun_cnt;
+
+	/* can produce response with up to 16k luns (lun 0 to lun 16383) */
+	n = min_t(int, SDEBUG_MAX_RLUNS, rlun_cnt);
+	if (n < rlun_cnt) {
+		wlun_cnt = 0;
 		lun_cnt = n;
 	}
-	one_lun = (struct scsi_lun *) &arr[8];
-	max_addr = arr + SDEBUG_RLUN_ARR_SZ;
-	for (i = 0, lun = (scsi_debug_no_lun_0 ? 1 : 0);
-             ((i < lun_cnt) && ((unsigned char *)(one_lun + i) < max_addr));
-	     i++, lun++) {
-		upper = (lun >> 8) & 0x3f;
-		if (upper)
-			one_lun[i].scsi_lun[0] =
-			    (upper | (SAM2_LUN_ADDRESS_METHOD << 6));
-		one_lun[i].scsi_lun[1] = lun & 0xff;
-	}
-	if (want_wlun) {
-		one_lun[i].scsi_lun[0] = (SCSI_W_LUN_REPORT_LUNS >> 8) & 0xff;
-		one_lun[i].scsi_lun[1] = SCSI_W_LUN_REPORT_LUNS & 0xff;
-		i++;
-	}
-	alloc_len = (unsigned char *)(one_lun + i) - arr;
-	return fill_from_dev_buffer(scp, arr,
-				    min((int)alloc_len, SDEBUG_RLUN_ARR_SZ));
+
+	pr_debug("select_report %d luns = %d wluns = %d no_lun0 %d\n",
+		 select_report, lun_cnt, wlun_cnt, scsi_debug_no_lun_0);
+
+	lun = 0LL;
+	/* luns start at offset 8 after the byte length */
+	one_lun = (struct scsi_lun *)&arr[8];
+
+	/* skip lun 0 */
+	if (scsi_debug_no_lun_0)
+		lun++;
+
+	/*
+	 * Address method (we use Peripherial = 00b)
+	 * 10b - Logical unit
+	 * 00b - Peripherial device - Use this one
+	 * 01b - Logical device
+	 * 11b - reserved
+	 */
+	for (i = 0; i < lun_cnt; i++)
+		int_to_scsilun(lun++, one_lun++);
+
+	/* report SCSI_W_LUN_REPORT_LUN */
+	if (wlun_cnt)
+		int_to_scsilun(SCSI_W_LUN_REPORT_LUNS, one_lun++);
+
+	rlen = rlun_cnt * sizeof(struct scsi_lun);
+
+	put_unaligned_be32(rlen, &arr[0]);
+
+	return fill_from_dev_buffer(scp, arr, rlen + 8);
 }
 
 static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
-- 
1.9.3

Re: [PATCH] scsi_debug: rework resp_report_luns

[Douglas Gilbert]

On 15-02-24 04:37 PM, Tomas Winkler wrote:
> 1. Fix the error check: the alloc length should be > 16
> and not > 4

You are proposing to make a marginally incorrect test
completely incorrect!

The governing rule for almost all "allocation length" fields
in SCSI commands (that return data-in) is:

"An allocation length of zero specifies that no data shall
be transferred. This condition shall not be considered an
error." [in section 4.2.5.6 of spc5r03.pdf and REPORT LUNS
refers to this]

If the REPORT LUNS allocation length is less that 4 then
the caller doesn't even get the response length back so that
has no practical use IMO. If allocation_length=0 and the SCSI
status is GOOD then all that tells you is that REPORT LUNS is
supported, but it has been a mandatory command for 10 years
so that is to be expected.

> 2. Remove duplicated boundary checks which simplify
> the fill-in loop
> 3. Use more of scsi generic API

Something else you might fix is the stack allocation of
2048+8 bytes:
    u8 arr[SDEBUG_RLUN_ARR_SZ];

It would be better to work out how many LUs the
parent target has and use kmalloc() (or friend)
to make a response buffer large enough. And
"large enough" needs to be no larger than the
allocation_length indicates however that requires
extra care in the loop stopping conditions.

Doug Gilbert

RE: [PATCH] scsi_debug: rework resp_report_luns

[Winkler, Tomas]

> On 15-02-24 04:37 PM, Tomas Winkler wrote:
> > 1. Fix the error check: the alloc length should be > 16
> > and not > 4
> 
> You are proposing to make a marginally incorrect test
> completely incorrect!

Quoting from the spec:
The ALLOCATION LENGTH field is defined in 2.2.6. The allocation length should be at least 16.
Note. Device servers compliant with SPC return CHECK CONDITION status, with the sense key set to
ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN CDB when the allocation
length is less than 16 bytes.
This is how it is implemented also in other drivers in the scsi drivers.

> 
> The governing rule for almost all "allocation length" fields
> in SCSI commands (that return data-in) is:
> 
> "An allocation length of zero specifies that no data shall
> be transferred. This condition shall not be considered an
> error." [in section 4.2.5.6 of spc5r03.pdf and REPORT LUNS
> refers to this]

Correct, but this wasn't the case also in the original code it checked for < 4.


> If the REPORT LUNS allocation length is less that 4 then
> the caller doesn't even get the response length back so that
> has no practical use IMO. If allocation_length=0 and the SCSI
> status is GOOD then all that tells you is that REPORT LUNS is
> supported, but it has been a mandatory command for 10 years
> so that is to be expected.

I believe that also original code doesn't handle that case.
 
> > 2. Remove duplicated boundary checks which simplify
> > the fill-in loop
> > 3. Use more of scsi generic API
> 
> Something else you might fix is the stack allocation of
> 2048+8 bytes:
>     u8 arr[SDEBUG_RLUN_ARR_SZ];
> 
> It would be better to work out how many LUs the
> parent target has and use kmalloc() (or friend)
> to make a response buffer large enough.

That might be fixed the question is if this not out of scope  for emulation  purposes.
To get spec limit to 16K maybe it will overcomplicate code uncessary. 
 
 And
> "large enough" needs to be no larger than the
> allocation_length indicates however that requires
> extra care in the loop stopping conditions.

We are not getting anything unexpected insde the loop, the boundaries are known before we entering the loop. 

Tomas

Re: [PATCH] scsi_debug: rework resp_report_luns

[Douglas Gilbert]

On 15-02-25 02:54 AM, Winkler, Tomas wrote:
>> On 15-02-24 04:37 PM, Tomas Winkler wrote:
>>> 1. Fix the error check: the alloc length should be > 16
>>> and not > 4
>>
>> You are proposing to make a marginally incorrect test
>> completely incorrect!
>
> Quoting from the spec:

The great thing about standards is that there are so many
to choose from :-)
So it is best to say _which_ spec.

> The ALLOCATION LENGTH field is defined in 2.2.6. The allocation length should be at least 16.
> Note. Device servers compliant with SPC return CHECK CONDITION status, with the sense key set to
> ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN CDB when the allocation
> length is less than 16 bytes.
> This is how it is implemented also in other drivers in the scsi drivers.

The scsi_debug driver contains this line:

#define DEF_SCSI_LEVEL   6    /* INQUIRY, byte2 [6->SPC-4] */

so by default it reports SPC-4 compliance. And what I have quoted
below is for SPC-4 and draft SPC-5.

My guess is that this was changed (between SPC-3 and SPC-4 ***)
for consistency (e.g. with LOG SENSE and INQUIRY) and flexibility.
It is awkward for an application client to set up a data-in
buffer for a size it does not yet know.

So if those "other drivers" report SPC-4 or later compliance
then they should be fixed since a REPORT LUNS allocation_length<16
is valid.

>> The governing rule for almost all "allocation length" fields
>> in SCSI commands (that return data-in) is:
>>
>> "An allocation length of zero specifies that no data shall
>> be transferred. This condition shall not be considered an
>> error." [in section 4.2.5.6 of spc5r03.pdf and REPORT LUNS
>> refers to this]
>
> Correct, but this wasn't the case also in the original code it checked for < 4.

>> If the REPORT LUNS allocation length is less that 4 then
>> the caller doesn't even get the response length back so that
>> has no practical use IMO. If allocation_length=0 and the SCSI
>> status is GOOD then all that tells you is that REPORT LUNS is
>> supported, but it has been a mandatory command for 10 years
>> so that is to be expected.
>
> I believe that also original code doesn't handle that case.

Correct, so if the app client sets an allocation length
of 3, then at your discretion, you can either leave the
code doing what it does now, or return those 3 bytes.
IOW leave it alone, improve it but don't make it worse.

>>> 2. Remove duplicated boundary checks which simplify
>>> the fill-in loop
>>> 3. Use more of scsi generic API
>>
>> Something else you might fix is the stack allocation of
>> 2048+8 bytes:
>>      u8 arr[SDEBUG_RLUN_ARR_SZ];
>>
>> It would be better to work out how many LUs the
>> parent target has and use kmalloc() (or friend)
>> to make a response buffer large enough.
>
> That might be fixed the question is if this not out of scope  for emulation  purposes.
> To get spec limit to 16K maybe it will overcomplicate code uncessary.
>
>   And
>> "large enough" needs to be no larger than the
>> allocation_length indicates however that requires
>> extra care in the loop stopping conditions.
>
> We are not getting anything unexpected insde the loop, the boundaries are known before we entering the loop.

I was noting that the boundary specified by the allocation
length (e.g. 3 or 11) may be an awkward fit for the loop
logic. For example you can't unconditionally use
put_unaligned_be32() into the response buffer if it was
kmalloc-ed for an allocation length of 3 bytes.

Doug Gilbert

*** The change was due to this T10 document: 11-004r0
     authored by Rob Elliott. It first appeared in spc4r20
     [20110331]

RE: [PATCH] scsi_debug: rework resp_report_luns

[Winkler, Tomas]

> Correct, so if the app client sets an allocation length
> of 3, then at your discretion, you can either leave the
> code doing what it does now, or return those 3 bytes.
> IOW leave it alone, improve it but don't make it worse.

Ack, got the new spec and looks like the check < 4 is the correct one as we cannot fulfill the command in less than ten 

'4.2.5.6 Allocation length
 If the amount of information that is available to be transferred exceeds the maximum value that the
ALLOCATION LENGTH field in combination with other fields in the CDB is capable of specifying, then no data
shall be transferred and the command shall be terminated with CHECK CONDITION status, with the sense
key set to ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN CDB.
'
The only special case that needs to be taken care of is check for == 0 which is not an error... just do not return anything? 

> 
> I was noting that the boundary specified by the allocation
> length (e.g. 3 or 11) may be an awkward fit for the loop
> logic. For example you can't unconditionally use
> put_unaligned_be32() into the response buffer if it was
> kmalloc-ed for an allocation length of 3 bytes.

That would not happen as we got out on < 4 and we use those 4 bytes to write the LUN LIST LENGTH 
Strangely the original code also ignored the allocation length from the request command and dumped whatever is available up to  SDEBUG_RLUN_ARR_SZ
That can be fixed too tough there some vague sentence: 
'The contents of the LUN LIST LENGTH field
are not altered based on the allocation length (see 4.2.5.6).'
 
Tomas

RE: [PATCH] scsi_debug: rework resp_report_luns

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Winkler, Tomas
> Sent: Wednesday, February 25, 2015 1:54 AM
> To: dgilbert@interlog.com; James E.J. Bottomley"
> Cc: linux-scsi@vger.kernel.org
> Subject: RE: [PATCH] scsi_debug: rework resp_report_luns
> 
> > On 15-02-24 04:37 PM, Tomas Winkler wrote:
> > > 1. Fix the error check: the alloc length should be > 16
> > > and not > 4
> >
> > You are proposing to make a marginally incorrect test
> > completely incorrect!
> 
> Quoting from the spec:
> The ALLOCATION LENGTH field is defined in 2.2.6. The allocation length
> should be at least 16.
> Note. Device servers compliant with SPC return CHECK CONDITION status,
> with the sense key set to ILLEGAL REQUEST, and the additional sense code
> set to INVALID FIELD IN CDB when the allocation length is less than 16 bytes.
> This is how it is implemented also in other drivers in the scsi drivers.

That was just a warning to software that very old devices (compliant with SPC-1)
returned CHECK CONDITION in that case, explaining why the "should" advice
is included.  New devices (SPC-2 and later) are not supposed to return
CHECK CONDITION.  "SPC" means SPC-1 only, not all versions.  This
warning note was in SPC-2 and SPC-3 and dropped in SPC-4.