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]
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
