PATCH: exclude certain commands from emulated SCSI hosts

PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

This patch changes how devices a probed on a SCSI bus if they are on an
emulated host.

A source-code survey shows that usb-storage, sbp2, ide-scsi, and the 3Ware
driver are the only 'emulated' hosts.

If a host is emulated, we (a) don't ask for EVPD data, as it's likely not
there, (b) assume that we have only 36-bytes of INQUIRY data, and (c) don't
ask disks for their cache-type (assume write-through).

All three of these things have been observed in the field to choke emulated
devices, thus it makes sense to cut them off at the SCSI layer.  In an
interesting side-note, this appears to be the only code which is
conditional on a host being emulated or not.

Linus, please apply to your 2.5.x tree.

Matthew Dharm

# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.697   -> 1.698  
#	   drivers/scsi/sd.c	1.47    -> 1.48   
#	drivers/scsi/scsi_scan.c	1.26    -> 1.27   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/03/22	mdharm@zen.san.one-eyed-alien.net	1.698
# Emulated hosts shouldn't get certain commands.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Sat Mar 22 19:19:22 2003
+++ b/drivers/scsi/scsi_scan.c	Sat Mar 22 19:19:22 2003
@@ -514,6 +514,10 @@
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	int max_lgth = 255;
 
+	/* emulated devices don't like the request for EVPD page */
+	if (sdev->host->hostt->emulated)
+		return NULL;
+
 retry:
 	evpd_page = kmalloc(max_lgth, GFP_ATOMIC |
 			      (sdev->host->unchecked_isa_dma) ?
@@ -851,6 +855,10 @@
 		return 0;
 	}
 
+	/* emulated devices do not react well to this sort of thing */
+	if (sdev->host->hostt->emulated)
+		goto leave;
+
 	memset(scsi_cmd, 0, MAX_COMMAND_SIZE);
 	scsi_cmd[0] = INQUIRY;
 	scsi_cmd[1] = 0x01;
@@ -1033,6 +1041,10 @@
 	 */
 	BUG_ON(bflags == NULL);
 	*bflags = scsi_get_device_flags(&inq_result[8], &inq_result[16]);
+
+	/* emulated devices should all be treated as blacklisted */
+	if (sdev->host->hostt->emulated)
+		*bflags |= BLIST_INQUIRY_36;
 
 	possible_inq_resp_len = (unsigned char) inq_result[4] + 5;
 	if (BLIST_INQUIRY_36 & *bflags)
diff -Nru a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c	Sat Mar 22 19:19:21 2003
+++ b/drivers/scsi/sd.c	Sat Mar 22 19:19:22 2003
@@ -1152,6 +1152,15 @@
 	const int dbd = 0x08;	   /* DBD */
 	const int modepage = 0x08; /* current values, cache page */
 
+	/* emulated devices likely do not support this type of mode sense */
+	if (sdkp->device->host->hostt->emulated) {
+		printk(KERN_NOTICE "%s: emulated, assuming drive cache: %s\n",
+		       diskname, "write through");
+		sdkp->WCE = 0;
+		sdkp->RCD = 0;
+		return;
+	}
+
 	/* cautiously ask */
 	res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer, 4);
 

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Sir, for the hundreth time, we do NOT carry 600-round boxes of belt-fed 
suction darts!
					-- Salesperson to Greg
User Friendly, 12/30/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Linus Torvalds]

On Sat, 22 Mar 2003, Matthew Dharm wrote:
>
> This patch changes how devices a probed on a SCSI bus if they are on an
> emulated host.

I really think this is wrong. I'd much much rather get _rid_ of that 
stupid "emulated" flag, instead of adding meaning to it.

> If a host is emulated, we (a) don't ask for EVPD data, as it's likely not
> there, (b) assume that we have only 36-bytes of INQUIRY data, and (c) don't
> ask disks for their cache-type (assume write-through).

..and thus anything that wants to emulate SCSI has to never be able to be
write-back again.

Now, I'd say that it is stupid to call yourself a SCSI disk if you aren't 
one _anyway_ (the raw block device interfaces are simpler and faster), but 
if you do, then I think it's doubly stupid to put in arbitrary 
restrictions like this.

If there are known commands that devices have trouble with (whether 
emulated or not), maybe we could have helper routines to do some default 
filtering, and have the queuecommand function check those. But this just 
looks ugly and hacky to me.

		Linus

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

On Sat, Mar 22, 2003 at 08:09:57PM -0800, Linus Torvalds wrote:
> 
> On Sat, 22 Mar 2003, Matthew Dharm wrote:
> >
> > This patch changes how devices a probed on a SCSI bus if they are on an
> > emulated host.
> 
> I really think this is wrong. I'd much much rather get _rid_ of that 
> stupid "emulated" flag, instead of adding meaning to it.

Well, you could just delete it.  It's not used anywhere.

> > If a host is emulated, we (a) don't ask for EVPD data, as it's likely not
> > there, (b) assume that we have only 36-bytes of INQUIRY data, and (c) don't
> > ask disks for their cache-type (assume write-through).
> 
> ..and thus anything that wants to emulate SCSI has to never be able to be
> write-back again.

Okay, I'll admit that could be a problem.

> Now, I'd say that it is stupid to call yourself a SCSI disk if you aren't 
> one _anyway_ (the raw block device interfaces are simpler and faster), but 
> if you do, then I think it's doubly stupid to put in arbitrary 
> restrictions like this.

The problem is this:  usb-storage interfaces via SCSI, but not just for
disks.  Tape, CD, etc. are all handled with the same protocol.  Heck, ATAPI
disk/cd/tape are handled with these code paths.

> If there are known commands that devices have trouble with (whether 
> emulated or not), maybe we could have helper routines to do some default 
> filtering, and have the queuecommand function check those. But this just 
> looks ugly and hacky to me.

Well, there are lots of 'known commands' that fall in to this category.  If
we had centralized helper functions, that would be great.  But, as it
stands, right now all the low-level drivers have to do all that separately,
and badly.

Right now, we've got nothing, which leaves low-level driver folks out in
the cold.

As I see it, SCSI commands break down into two basic categories:  common
and uncommon.  Common things (basic read and write, 36-byte INQUIRY, eject,
etc.) are all fine, but the 'uncommon' things (checking cache type,
255-byte INQUIRY, etc) cause problems.  I'm trying to find a way to choke
off the problematic commands without having to write code to recognize what
is being sent (and choke it off) based on what is in the command bytes.

I'm open to suggestions.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Linus Torvalds]

On Sat, 22 Mar 2003, Matthew Dharm wrote:
> 
> As I see it, SCSI commands break down into two basic categories:  common
> and uncommon.  Common things (basic read and write, 36-byte INQUIRY, eject,
> etc.) are all fine, but the 'uncommon' things (checking cache type,
> 255-byte INQUIRY, etc) cause problems.  I'm trying to find a way to choke
> off the problematic commands without having to write code to recognize what
> is being sent (and choke it off) based on what is in the command bytes.
> 
> I'm open to suggestions.

How about making the SCSI stuff pass a "common" flag (or "required") down
with the command? Then, a emulated thing could just decide to punt all 
commands with an immediate failure if they aren't marked "required".

That still _allows_ the driver to implement it if it wants to, unlike your 
previous approach.

		Linus

Re: [usb-storage] Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Sat, Mar 22, 2003 at 11:39:05PM -0800, Linus Torvalds wrote:
> How about making the SCSI stuff pass a "common" flag (or "required") down
> with the command? Then, a emulated thing could just decide to punt all 
> commands with an immediate failure if they aren't marked "required".
> 
> That still _allows_ the driver to implement it if it wants to, unlike your 
> previous approach.

That seems reasonable... but we need to define a standard way to
reject/fail non-common commands.  Special sense data?  Special result
code?  It doesn't really matter to me, but we need to pick something.

Do you have a preference?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [usb-storage] Re: PATCH: exclude certain commands from emulated SCSI hosts

[Douglas Gilbert]

Matthew Dharm wrote:
> On Sat, Mar 22, 2003 at 11:39:05PM -0800, Linus Torvalds wrote:
> 
>>How about making the SCSI stuff pass a "common" flag (or "required") down
>>with the command? Then, a emulated thing could just decide to punt all 
>>commands with an immediate failure if they aren't marked "required".
>>
>>That still _allows_ the driver to implement it if it wants to, unlike your 
>>previous approach.
> 
> 
> That seems reasonable... but we need to define a standard way to
> reject/fail non-common commands.  Special sense data?  Special result
> code?  It doesn't really matter to me, but we need to pick something.
> 
> Do you have a preference?

Matt,
Evidentally your driver (and/or sbp2) synthesizes INQUIRY
responses in some cases. So if you think a SCSI command
is risky then why not follow the standard and synthesize
the scsi status and auto-sense buffer. Here is a modern
rendition from a (draft) standard:

"If a device server receives a CDB containing an operation
code that is invalid or not supported, it shall return
CHECK CONDITION status with the sense key set to ILLEGAL
REQUEST and an additional sense code of INVALID COMMAND
OPERATION CODE." [SPC-3, T10/1416-D Revision 10 2002/11/10]

If it is a field within a command that is in doubt (e.g.
the enable Vital Product Data (EVPD) bit in an INQUIRY)
then the additional sense code becomes INVALID FIELD IN
CDB.

In the case of a disk the commands that must be
supported are:
    - 36 byte INQUIRY (EVPD=0, CmdDt=0)
    - READ CAPACITY
    - READ (10 byte)
    - WRITE (10 byte)

For SCSI mid level scan code and the sd
disk initialization code a flag could be added to
struct scsi_cmnd to indicate that "I can live without
knowing the answer to this command".

I always wanted the emulated flag generalized (e.g.
indicating SPI, ATAPI, iSCSI, FC, SAS, SBP-2,
USB_mass_storage). Knowledge of the SCSI transport
protocol is important for device discovery,
identification and asynchronous notification sources.
As a boolean flag, "emulated" has outlived its usefulness.
With sysfs we can now find which hardware system a
SCSI low level driver belongs  to (e.g. SCSI Parallel
Interface (SPI) adapters are either PCI or ISA).

Doug Gilbert

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Sun, 2003-03-23 at 01:31, Matthew Dharm wrote:
> The problem is this:  usb-storage interfaces via SCSI, but not just for
> disks.  Tape, CD, etc. are all handled with the same protocol.  Heck, ATAPI
> disk/cd/tape are handled with these code paths.

ATAPI isn't really "emulated" in the sense that ATAPI devices understand
a subset of SCSI commands (albeit according to their own rules).

I'll give you the six byte mode sense, since ATAPI devices only do the
ten byte version, but they nevertheless understand it.

> > If there are known commands that devices have trouble with (whether 
> > emulated or not), maybe we could have helper routines to do some default 
> > filtering, and have the queuecommand function check those. But this just 
> > looks ugly and hacky to me.
> 
> Well, there are lots of 'known commands' that fall in to this category.  If
> we had centralized helper functions, that would be great.  But, as it
> stands, right now all the low-level drivers have to do all that separately,
> and badly.

The problem with this type of approach is that there's no unified list
of "known good" commands that actually let you operate a device.  The
SCSI and ATAPI standards have been gradually deprecating the commands
that SCSI-1 (and sometimes even SCSI-2) regard as mandatory
(READ_6/WRITE_6 springs to mind).

Given this, the upper level drivers have to try playing a bit to see
what the device will actually support.

> Right now, we've got nothing, which leaves low-level driver folks out in
> the cold.

Well, for truly "emulated" (meaning the command is interpreted inside
the driver and executed there instead of simply being repackaged and
passed on to the device), a library of helper functions would be
acceptable to place in the mid-layer if you want to write such a thing.

> As I see it, SCSI commands break down into two basic categories:  common
> and uncommon.  Common things (basic read and write, 36-byte INQUIRY, eject,
> etc.) are all fine, but the 'uncommon' things (checking cache type,
> 255-byte INQUIRY, etc) cause problems.  I'm trying to find a way to choke
> off the problematic commands without having to write code to recognize what
> is being sent (and choke it off) based on what is in the command bytes.

Under any definition of "common", a device that can't accept variable
length inquiry commands is broken (by every known standard, even). 
However, lets not rehash old sore points.

Could you elaborate on what the "problem" is.  There seem to be two
separate issues:

1. True devices which have incomplete state models for SCSI inside the
real device and thus react badly to certain commands.  For this one, I'm
happy to have a helper library containing the elements of the SCSI state
model for the devices to use, but I do believe that the drivers are
broken and need fixing.

2. Certain real SCSI (or ATAPI) devices which have broken internal SCSI
processing models.  For these we need to not send certain commands which
annoy them.  This is much harder, since I don't believe we'll get a
common definition of annoying commands that will work for every device.

For ATAPI, we can probably get pretty far with a "no six byte commands"
flag. This should be fairly easy to implement since every LLD tends to
know when it is repackaging for an ATAPI device.  What other rules would
you need for this type of thing?

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Sun, Mar 23, 2003 at 07:26:09PM -0600, James Bottomley wrote:
> The problem with this type of approach is that there's no unified list
> of "known good" commands that actually let you operate a device.  The
> SCSI and ATAPI standards have been gradually deprecating the commands
> that SCSI-1 (and sometimes even SCSI-2) regard as mandatory
> (READ_6/WRITE_6 springs to mind).

Actually, there is such a list.  It's the commands that the 'popular OS'
uses, and I have a pretty good idea exactly what those are.  That's why my
original approach was to just cut out the commands that fell outside that
definition.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I want my GPFs!!!
					-- Stef
User Friendly, 11/9/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Sun, 2003-03-23 at 19:37, Matthew Dharm wrote:
> Actually, there is such a list.  It's the commands that the 'popular OS'
> uses, and I have a pretty good idea exactly what those are.  That's why my
> original approach was to just cut out the commands that fell outside that
> definition.

Well, if you want to write a helper for the mid layer that checks the
commands and returns a Check Condition with sense Illegal Reguest on the
bad ones, that sounds like the best approach.  You can then call the
helper function in the queuecommand of the problem emulated drivers.

Can you publish a rough list of these?

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

Well, here's my list of what the 'popular OS' uses for all devices:

INQUIRY (for only 36 bytes -- nothing else!)
TEST_UNIT_READY
REQUEST_SENSE
ALLOW_MEDIUM_REMOVAL (mostly for eject purposes)

For disk-like media:

READ_10
WRITE_10

I'd have to go back to my notes for tape and CD media.  But those aren't my
biggest problem areas -- the common probe and the disk driver are my
biggest headaches.

Note that MODE_SENSE isn't on this list.  How does the 'popular OS' test
for write-protect, you ask?  It tries to write and then looks for a
failure, AFAICT.

Note that this data comes from observations about what commands all devices
seem to support.

I'd be willing to write a helper, but I'm a bit out of my element here...
can someone at least suggest a good place to put such a helper (or
volunteer to mock one up for me)?

Matt

On Sun, Mar 23, 2003 at 07:39:01PM -0600, James Bottomley wrote:
> On Sun, 2003-03-23 at 19:37, Matthew Dharm wrote:
> > Actually, there is such a list.  It's the commands that the 'popular OS'
> > uses, and I have a pretty good idea exactly what those are.  That's why my
> > original approach was to just cut out the commands that fell outside that
> > definition.
> 
> Well, if you want to write a helper for the mid layer that checks the
> commands and returns a Check Condition with sense Illegal Reguest on the
> bad ones, that sounds like the best approach.  You can then call the
> helper function in the queuecommand of the problem emulated drivers.
> 
> Can you publish a rough list of these?
> 
> James
> 

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Dudes! May the Open Source be with you.
					-- Eric S. Raymond
User Friendly, 12/3/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-03-24 at 01:04, Matthew Dharm wrote:
> Well, here's my list of what the 'popular OS' uses for all devices:
> 
> INQUIRY (for only 36 bytes -- nothing else!)
> TEST_UNIT_READY
> REQUEST_SENSE
> ALLOW_MEDIUM_REMOVAL (mostly for eject purposes)
> 
> For disk-like media:
> 
> READ_10
> WRITE_10

We do about the best we can for read and write.  For sd, we gauge the
size of the command from the size of the medium:  <1Gb=> six byte, from
1Gb to 2Tb 10 byte, over 2Tb 16 byte, so I think this should all be
fine.

> I'd have to go back to my notes for tape and CD media.  But those aren't my
> biggest problem areas -- the common probe and the disk driver are my
> biggest headaches.
> 
> Note that MODE_SENSE isn't on this list.  How does the 'popular OS' test
> for write-protect, you ask?  It tries to write and then looks for a
> failure, AFAICT.

We use MODE_SENSE on CDs to probe for capabilities: this is required
behaviour and we've been doing for a long time.

sd also uses MODE_SENSE to probe the cache type as well as the write
protect state.  Cache type certainly can't be obtained any other way,
and I'm not sure allowing writes to read only media wouldn't cause us
more problems in the long run.

> I'd be willing to write a helper, but I'm a bit out of my element here...
> can someone at least suggest a good place to put such a helper (or
> volunteer to mock one up for me)?

OK, I can do this: A simple one with either a blacklist (reject these
commands) or whitelist (only accept these commands) going by the first
command byte OK?

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Linus Torvalds]

On 24 Mar 2003, James Bottomley wrote:
> > 
> > For disk-like media:
> > 
> > READ_10
> > WRITE_10
> 
> We do about the best we can for read and write.  For sd, we gauge the
> size of the command from the size of the medium:  <1Gb=> six byte, from
> 1Gb to 2Tb 10 byte, over 2Tb 16 byte, so I think this should all be
> fine.

Really? The code was _supposed_ to always start off with READ/WRITE_10's,
and then fall back to the old READ/WRITE_6 if it gets errors from that. Do 
we really have some broken random-number generator semantic still in teh 
SCSI layer? That sounds like a piece of crock.

		Linus

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-03-24 at 10:29, Linus Torvalds wrote:
> Really? The code was _supposed_ to always start off with READ/WRITE_10's,
> and then fall back to the old READ/WRITE_6 if it gets errors from that. Do 
> we really have some broken random-number generator semantic still in teh 
> SCSI layer? That sounds like a piece of crock.

Well, OK, the whole story is that sd does size the command according to
the request, as I said before.  However, if the six byte command fails
with an illegal request sense then we'll retry it at ten bytes and lock
the device to accept only ten byte commands using the ten flag in the
struct scsi_device.

We never do sixteen byte commands unless a region of the device >2Tb is
accessed (in which case the device must support sixteen byte commands).

I think this behaviour is reasonably optimal.  I suspect more ancient
SCSI-1 devices would have issues with READ/WRITE_10 than modern devices
that don't do READ_6 but also are unable to return the correct error
code.

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Jens Axboe]

On Mon, Mar 24 2003, Linus Torvalds wrote:
> 
> On 24 Mar 2003, James Bottomley wrote:
> > > 
> > > For disk-like media:
> > > 
> > > READ_10
> > > WRITE_10
> > 
> > We do about the best we can for read and write.  For sd, we gauge the
> > size of the command from the size of the medium:  <1Gb=> six byte, from
> > 1Gb to 2Tb 10 byte, over 2Tb 16 byte, so I think this should all be
> > fine.
> 
> Really? The code was _supposed_ to always start off with READ/WRITE_10's,
> and then fall back to the old READ/WRITE_6 if it gets errors from that. Do 
> we really have some broken random-number generator semantic still in teh 
> SCSI layer? That sounds like a piece of crock.

It's not true, ->ten is set unconditionally and we only fall back to 6
byte cdb's if we see an ILLEGAL_REQUEST on a READ_10/WRITE_10.

So the logic is, always assume 10-byte commands. If an incoming request
cannot be addressed with 10-byte commands, use 16.

-- 
Jens Axboe

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-03-24 at 10:52, Jens Axboe wrote:
> It's not true, ->ten is set unconditionally and we only fall back to 6
> byte cdb's if we see an ILLEGAL_REQUEST on a READ_10/WRITE_10.
> 
> So the logic is, always assume 10-byte commands. If an incoming request
> cannot be addressed with 10-byte commands, use 16.

Sorry, that was me misreading the fallback logic in sd.c

Ten bytes it is unless we have problems.

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

On Mon, Mar 24, 2003 at 09:15:57AM -0600, James Bottomley wrote:
> On Mon, 2003-03-24 at 01:04, Matthew Dharm wrote:
> > Note that MODE_SENSE isn't on this list.  How does the 'popular OS' test
> > for write-protect, you ask?  It tries to write and then looks for a
> > failure, AFAICT.
> 
> We use MODE_SENSE on CDs to probe for capabilities: this is required
> behaviour and we've been doing for a long time.
> 
> sd also uses MODE_SENSE to probe the cache type as well as the write
> protect state.  Cache type certainly can't be obtained any other way,
> and I'm not sure allowing writes to read only media wouldn't cause us
> more problems in the long run.

CD is fine, I was referring to MODE_SENSE in sd.c

I think allowing write to read-only media is the only way to go.  I don't
see another way to get write-protect status.

> > I'd be willing to write a helper, but I'm a bit out of my element here...
> > can someone at least suggest a good place to put such a helper (or
> > volunteer to mock one up for me)?
> 
> OK, I can do this: A simple one with either a blacklist (reject these
> commands) or whitelist (only accept these commands) going by the first
> command byte OK?

Well, you need to go by more than the first command byte -- ex. INQUIRY is
okay, unless length != 36 or EVPD.

I think a blacklist is probably in order, but with a BIG COMMENT mentioning
that if someone adds new commands into the code paths they should at least
consider if they belong in the blacklist.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Type "format c:"  That should fix everything.
					-- Greg
User Friendly, 12/18/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Mon, 2003-03-24 at 11:30, Matthew Dharm wrote:
> On Mon, Mar 24, 2003 at 09:15:57AM -0600, James Bottomley wrote:
> > OK, I can do this: A simple one with either a blacklist (reject these
> > commands) or whitelist (only accept these commands) going by the first
> > command byte OK?
> 
> Well, you need to go by more than the first command byte -- ex. INQUIRY is
> okay, unless length != 36 or EVPD.
> 
> I think a blacklist is probably in order, but with a BIG COMMENT mentioning
> that if someone adds new commands into the code paths they should at least
> consider if they belong in the blacklist.

OK, try the attached.  There is no central blacklist, you must construct
it on a per driver basis, so in the queuecommand of your driver you have
a:

	static struct scsi_cmd_filter filter =
		{ SCSI_FILTER_BLACKLIST ,
		  { MODE_SENSE, SCSI_FILTER_INQUIRY_NOT36, 0 } };

	if(scsi_filter_cmd(SCp, &filter)) {
		SCp->scsi_done(SCp);
		return 0;
	}

to use the filter (I think, I've compiled but not tested it).

Let me know how it goes.

James


[-- Attachment #2: scsi_filter.diff --]
[-- Type: text/plain, Size: 3642 bytes --]

===== drivers/scsi/scsi.h 1.68 vs edited =====
--- 1.68/drivers/scsi/scsi.h	Mon Mar 24 11:23:37 2003
+++ edited/drivers/scsi/scsi.h	Sat Apr  5 09:24:38 2003
@@ -959,4 +959,39 @@
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 
+struct scsi_cmd_filter {
+	enum {
+		SCSI_FILTER_WHITELIST,
+		SCSI_FILTER_BLACKLIST
+	} type;
+	/* normal scsi commands are bytes, exceptions are words.  The format
+	 * of the exceptions is:
+	 *
+	 * Byte 15: invert the specific condition
+	 * Byte 14-12: Reserved
+	 * Byte 11-8: command opcode (must be > 0)
+	 * Byte 7-0: Command specific */
+	unsigned short commands[];
+};
+
+extern int scsi_filter_cmd(struct scsi_cmnd *, struct scsi_cmd_filter *);
+
+/* exception definitions for the filter */
+#define SCSI_FILTER_INVERT		0x8000
+
+/* opcodes for the filter */
+#define SCSI_FILTER_INQUIRY		0x0100
+
+/* useful filter commands */
+#define SCSI_FILTER_INQUIRY_36		(SCSI_FILTER_INQUIRY | 36)
+#define SCSI_FILTER_INQUIRY_NOT36	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY | 36)
+
+static inline unsigned short scsi_filter_opcode(unsigned short command) {
+	return command & 0x7f00;
+}
+
+static inline unsigned char scsi_filter_data(unsigned short command) {
+	return (unsigned char)(command & 0x00ff);
+}
+
 #endif /* _SCSI_H */
===== drivers/scsi/scsi_lib.c 1.75 vs edited =====
--- 1.75/drivers/scsi/scsi_lib.c	Fri Mar 14 18:35:27 2003
+++ edited/drivers/scsi/scsi_lib.c	Sat Apr  5 09:27:35 2003
@@ -1375,3 +1375,71 @@
 		kmem_cache_destroy(sgp->slab);
 	}
 }
+
+static inline int scsi_filter_exceptions(struct scsi_cmnd *cmd,
+					 unsigned short command)
+{
+	int found = 0;
+
+	/* specials begin at 0x100 */
+	if(command < 0x100)
+		return 0;
+
+	switch(scsi_filter_opcode(command)) {
+
+	case SCSI_FILTER_INQUIRY:
+		if(cmd->cmnd[0] != INQUIRY)
+			return 0;
+		/* does the transfer length match the data */
+		found = (cmd->cmnd[4] == scsi_filter_data(command));
+		/* now check for inversion */
+		if(command & SCSI_FILTER_INVERT)
+			found = !found;
+		return found;
+	default:
+		/* unrecognized filter */
+		return 0;
+	}
+}
+
+/**
+ * scsi_filter_cmd - Filter a given command against a list
+ * @cmd: command to be filtered.
+ * @filter: pointer to the filter containing the type (black/white list) and
+ * zero terminated list of commands to filter against (first byte only).
+ *
+ * Returns 0 if the filter passed successfully and the driver can continue
+ * processing the command or 1 if the filter failed and the command should
+ * be finished (via ->scsi_done).  In the latter case, the command will have
+ * the sense fields filled in indicating the correct sense for an illegal
+ * request.
+ **/
+int scsi_filter_cmd(struct scsi_cmnd *cmd, struct scsi_cmd_filter *filter)
+{
+	int found = 0;
+	unsigned short *command;
+
+	for(command = filter->commands; *command; command++) {
+		found = scsi_filter_exceptions(cmd, *command);
+		found += (cmd->cmnd[0] != *command);
+		if(found)
+			break;
+	}
+
+	if((found && filter->type == SCSI_FILTER_WHITELIST) ||
+	   (!found && filter->type == SCSI_FILTER_BLACKLIST))
+		return 0;
+
+	/* fill in the Check Condition/Illegal request */
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	memset(cmd->sense_buffer, '\0', sizeof(cmd->sense_buffer));
+	cmd->sense_buffer[0] = 0xF0; /* valid, response code 0x70 */
+	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
+
+	/* ASC 0x20, ASCQ 0x00: Invalid command operation code */
+	cmd->sense_buffer[12] = 0x20;
+	cmd->sense_buffer[13] = 0x00;
+
+	return 1;
+}
+	

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 5834 bytes --]

I don't think this will work.  I see two problems:

(1) You can't filter TEST_UNIT_READY (opcode 0).  Not a big deal, but a
theoretical problem.

(2) We need to be able to filter at the originator.  For example,
MODE_SENSE is perfectly fine to send to a CD-ROM, but not to a hard-disk.
We can't make that distinction with your code, unless we put the filtering
code not in queuecommand but in places like sd.c.... or we need to change
the filter to also take a device type.

Matt

On Sat, Apr 05, 2003 at 09:30:41AM -0600, James Bottomley wrote:
> On Mon, 2003-03-24 at 11:30, Matthew Dharm wrote:
> > On Mon, Mar 24, 2003 at 09:15:57AM -0600, James Bottomley wrote:
> > > OK, I can do this: A simple one with either a blacklist (reject these
> > > commands) or whitelist (only accept these commands) going by the first
> > > command byte OK?
> > 
> > Well, you need to go by more than the first command byte -- ex. INQUIRY is
> > okay, unless length != 36 or EVPD.
> > 
> > I think a blacklist is probably in order, but with a BIG COMMENT mentioning
> > that if someone adds new commands into the code paths they should at least
> > consider if they belong in the blacklist.
> 
> OK, try the attached.  There is no central blacklist, you must construct
> it on a per driver basis, so in the queuecommand of your driver you have
> a:
> 
> 	static struct scsi_cmd_filter filter =
> 		{ SCSI_FILTER_BLACKLIST ,
> 		  { MODE_SENSE, SCSI_FILTER_INQUIRY_NOT36, 0 } };
> 
> 	if(scsi_filter_cmd(SCp, &filter)) {
> 		SCp->scsi_done(SCp);
> 		return 0;
> 	}
> 
> to use the filter (I think, I've compiled but not tested it).
> 
> Let me know how it goes.
> 
> James
> 

> ===== drivers/scsi/scsi.h 1.68 vs edited =====
> --- 1.68/drivers/scsi/scsi.h	Mon Mar 24 11:23:37 2003
> +++ edited/drivers/scsi/scsi.h	Sat Apr  5 09:24:38 2003
> @@ -959,4 +959,39 @@
>  extern int scsi_sysfs_register(void);
>  extern void scsi_sysfs_unregister(void);
>  
> +struct scsi_cmd_filter {
> +	enum {
> +		SCSI_FILTER_WHITELIST,
> +		SCSI_FILTER_BLACKLIST
> +	} type;
> +	/* normal scsi commands are bytes, exceptions are words.  The format
> +	 * of the exceptions is:
> +	 *
> +	 * Byte 15: invert the specific condition
> +	 * Byte 14-12: Reserved
> +	 * Byte 11-8: command opcode (must be > 0)
> +	 * Byte 7-0: Command specific */
> +	unsigned short commands[];
> +};
> +
> +extern int scsi_filter_cmd(struct scsi_cmnd *, struct scsi_cmd_filter *);
> +
> +/* exception definitions for the filter */
> +#define SCSI_FILTER_INVERT		0x8000
> +
> +/* opcodes for the filter */
> +#define SCSI_FILTER_INQUIRY		0x0100
> +
> +/* useful filter commands */
> +#define SCSI_FILTER_INQUIRY_36		(SCSI_FILTER_INQUIRY | 36)
> +#define SCSI_FILTER_INQUIRY_NOT36	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY | 36)
> +
> +static inline unsigned short scsi_filter_opcode(unsigned short command) {
> +	return command & 0x7f00;
> +}
> +
> +static inline unsigned char scsi_filter_data(unsigned short command) {
> +	return (unsigned char)(command & 0x00ff);
> +}
> +
>  #endif /* _SCSI_H */
> ===== drivers/scsi/scsi_lib.c 1.75 vs edited =====
> --- 1.75/drivers/scsi/scsi_lib.c	Fri Mar 14 18:35:27 2003
> +++ edited/drivers/scsi/scsi_lib.c	Sat Apr  5 09:27:35 2003
> @@ -1375,3 +1375,71 @@
>  		kmem_cache_destroy(sgp->slab);
>  	}
>  }
> +
> +static inline int scsi_filter_exceptions(struct scsi_cmnd *cmd,
> +					 unsigned short command)
> +{
> +	int found = 0;
> +
> +	/* specials begin at 0x100 */
> +	if(command < 0x100)
> +		return 0;
> +
> +	switch(scsi_filter_opcode(command)) {
> +
> +	case SCSI_FILTER_INQUIRY:
> +		if(cmd->cmnd[0] != INQUIRY)
> +			return 0;
> +		/* does the transfer length match the data */
> +		found = (cmd->cmnd[4] == scsi_filter_data(command));
> +		/* now check for inversion */
> +		if(command & SCSI_FILTER_INVERT)
> +			found = !found;
> +		return found;
> +	default:
> +		/* unrecognized filter */
> +		return 0;
> +	}
> +}
> +
> +/**
> + * scsi_filter_cmd - Filter a given command against a list
> + * @cmd: command to be filtered.
> + * @filter: pointer to the filter containing the type (black/white list) and
> + * zero terminated list of commands to filter against (first byte only).
> + *
> + * Returns 0 if the filter passed successfully and the driver can continue
> + * processing the command or 1 if the filter failed and the command should
> + * be finished (via ->scsi_done).  In the latter case, the command will have
> + * the sense fields filled in indicating the correct sense for an illegal
> + * request.
> + **/
> +int scsi_filter_cmd(struct scsi_cmnd *cmd, struct scsi_cmd_filter *filter)
> +{
> +	int found = 0;
> +	unsigned short *command;
> +
> +	for(command = filter->commands; *command; command++) {
> +		found = scsi_filter_exceptions(cmd, *command);
> +		found += (cmd->cmnd[0] != *command);
> +		if(found)
> +			break;
> +	}
> +
> +	if((found && filter->type == SCSI_FILTER_WHITELIST) ||
> +	   (!found && filter->type == SCSI_FILTER_BLACKLIST))
> +		return 0;
> +
> +	/* fill in the Check Condition/Illegal request */
> +	cmd->result = SAM_STAT_CHECK_CONDITION;
> +	memset(cmd->sense_buffer, '\0', sizeof(cmd->sense_buffer));
> +	cmd->sense_buffer[0] = 0xF0; /* valid, response code 0x70 */
> +	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
> +
> +	/* ASC 0x20, ASCQ 0x00: Invalid command operation code */
> +	cmd->sense_buffer[12] = 0x20;
> +	cmd->sense_buffer[13] = 0x00;
> +
> +	return 1;
> +}
> +	


-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

IT KEEPS ASKING ME WHERE I WANT TO GO TODAY! I DONT WANT TO GO ANYWHERE!
					-- Greg
User Friendly, 11/28/97

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Sat, 2003-04-05 at 13:27, Matthew Dharm wrote:
> I don't think this will work.  I see two problems:
> 
> (1) You can't filter TEST_UNIT_READY (opcode 0).  Not a big deal, but a
> theoretical problem.

Yes, TUR has been mandatory since SCSI-1, so I didn't think this would
be a problem.  Any end signal can be used, zero was the least line of
resistance.

> (2) We need to be able to filter at the originator.  For example,
> MODE_SENSE is perfectly fine to send to a CD-ROM, but not to a hard-disk.
> We can't make that distinction with your code, unless we put the filtering
> code not in queuecommand but in places like sd.c.... or we need to change
> the filter to also take a device type.

What's wrong with

-		if(scsi_filter_cmd(SCp, &filter)) {
+		if(SCp->device->type == TYPE_DISK && scsi_filter_cmd(SCp, &filter)) {

?

The whole idea is to provide a filter library that the emulated HBA
drivers can use to cope with standard commands they don't like rather
than add extra code to the mid-layer main code paths.

James




-------------------------------------------------------
This SF.net email is sponsored by: ValueWeb: 
Dedicated Hosting for just $79/mo with 500 GB of bandwidth! 
No other company gives more support or power for your dedicated server
http://click.atdmt.com/AFF/go/sdnxxaff00300020aff/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

On Sat, Apr 05, 2003 at 01:45:43PM -0600, James Bottomley wrote:
> On Sat, 2003-04-05 at 13:27, Matthew Dharm wrote:
> > I don't think this will work.  I see two problems:
> > 
> > (1) You can't filter TEST_UNIT_READY (opcode 0).  Not a big deal, but a
> > theoretical problem.
> 
> Yes, TUR has been mandatory since SCSI-1, so I didn't think this would
> be a problem.  Any end signal can be used, zero was the least line of
> resistance.

For the sake of completeness, I think I'd rather see something else used --
perhaps something in the high-byte (where you currently store the inversion
bit).

> > (2) We need to be able to filter at the originator.  For example,
> > MODE_SENSE is perfectly fine to send to a CD-ROM, but not to a hard-disk.
> > We can't make that distinction with your code, unless we put the filtering
> > code not in queuecommand but in places like sd.c.... or we need to change
> > the filter to also take a device type.
> 
> What's wrong with
> 
> -		if(scsi_filter_cmd(SCp, &filter)) {
> +		if(SCp->device->type == TYPE_DISK && scsi_filter_cmd(SCp, &filter)) {
> 
> ?
> 
> The whole idea is to provide a filter library that the emulated HBA
> drivers can use to cope with standard commands they don't like rather
> than add extra code to the mid-layer main code paths.

My big complaint about that is that it's ugly.  I would like to keep the
device type as part of the filter command structure, so I can keep the call
to scsi_filter_cmd() simple and easy to maintain.  Maybe it's just me, but
I think that editing the filter table with an extra field will be easier to
get right than trying to maintain several different filter tables (for each
device type).

I guess the real power of this filter is in the ability to add logic to
scsi_filter_exceptions()... but centralizing that seems contrary to the
idea of doing this on a per-HBA basis.

What about a filter table of structs with 3 fields -- white/blacklist, type
to apply to, and function pointer to something that returns 0 or 1?  We can
provide some of the 'test functions' in the core for those who want it
(i.e. provide something that implements the 36-byte INQUIRY restriction),
but each HBA can create an arbitrary list of their own as well.

With something like that, we might even be able to collapse some of the
current SCSI device blacklisting into this mechanism.  But that's just a
thought for the future.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It's not that hard.  No matter what the problem is, tell the customer 
to reinstall Windows.
					-- Nurse
User Friendly, 3/22/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Sat, 2003-04-05 at 13:55, Matthew Dharm wrote:
> 
> My big complaint about that is that it's ugly.  I would like to keep the
> device type as part of the filter command structure, so I can keep the call
> to scsi_filter_cmd() simple and easy to maintain.  Maybe it's just me, but
> I think that editing the filter table with an extra field will be easier to
> get right than trying to maintain several different filter tables (for each
> device type).

But that would reduce the power of the filter.  At the moment you can
code "all devices apart from tapes" or "only disk and cdrom".  If I add
it to the body of the filter, I either have to add a complex language to
express this or reduce the power.  Neither seems to be particularly
optimal

> I guess the real power of this filter is in the ability to add logic to
> scsi_filter_exceptions()... but centralizing that seems contrary to the
> idea of doing this on a per-HBA basis.

The filter is just a language.  HBAs don't need to use all the
expressions in the exceptions, so unless it gets too big I don't see why
necessary additions can't go centrally.

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Sat, Apr 05, 2003 at 02:08:29PM -0600, James Bottomley wrote:
> On Sat, 2003-04-05 at 13:55, Matthew Dharm wrote:
> > 
> > My big complaint about that is that it's ugly.  I would like to keep the
> > device type as part of the filter command structure, so I can keep the call
> > to scsi_filter_cmd() simple and easy to maintain.  Maybe it's just me, but
> > I think that editing the filter table with an extra field will be easier to
> > get right than trying to maintain several different filter tables (for each
> > device type).
> 
> But that would reduce the power of the filter.  At the moment you can
> code "all devices apart from tapes" or "only disk and cdrom".  If I add
> it to the body of the filter, I either have to add a complex language to
> express this or reduce the power.  Neither seems to be particularly
> optimal

Okay, I'll buy that.

> > I guess the real power of this filter is in the ability to add logic to
> > scsi_filter_exceptions()... but centralizing that seems contrary to the
> > idea of doing this on a per-HBA basis.
> 
> The filter is just a language.  HBAs don't need to use all the
> expressions in the exceptions, so unless it gets too big I don't see why
> necessary additions can't go centrally.

Eh.  I'm not really convinced on this point (based on my past history of
trying to get patches into the SCSI subsystem), but I think that situation
is improving, so I'll just let it go.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

God, root, what is difference?
					-- Pitr
User Friendly, 11/11/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Sat, Apr 05, 2003 at 01:45:43PM -0600, James Bottomley wrote:
> What's wrong with
> 
> -		if(scsi_filter_cmd(SCp, &filter)) {
> +		if(SCp->device->type == TYPE_DISK && scsi_filter_cmd(SCp, &filter)) {
> 
> ?

Out of curiosity, at what point is SCp->device->type a valid reference?  I
would hope that none of those pointers would be NULL during a probe at init
time.  I also hope that the type field contains the right data as soon as
the INQUIRY data comes back... but does anyone actually know for certain?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Pitr!  That's brilliant!  Funded by Microsoft refunds.  What sweet irony!
					-- A.J.
User Friendly, 2/15/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Sat, 2003-04-05 at 18:22, Matthew Dharm wrote:
> Out of curiosity, at what point is SCp->device->type a valid reference?  I
> would hope that none of those pointers would be NULL during a probe at init
> time.  I also hope that the type field contains the right data as soon as
> the INQUIRY data comes back... but does anyone actually know for certain?

sdev->device is never NULL for an executing command.  Type is filled in
after the first 36 byte inquiry goes to the device.

James




-------------------------------------------------------
This SF.net email is sponsored by: ValueWeb: 
Dedicated Hosting for just $79/mo with 500 GB of bandwidth! 
No other company gives more support or power for your dedicated server
http://click.atdmt.com/AFF/go/sdnxxaff00300020aff/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Patrick Mansfield]

On Sat, Apr 05, 2003 at 09:30:41AM -0600, James Bottomley wrote:
> On Mon, 2003-03-24 at 11:30, Matthew Dharm wrote:

James/Matt -

Some specific questions:

Why not just per command filtering? So either the adapter driver (or
its underlying transport) supports a command or it does not. Other cases
would need other b/w list support.

Otherwise, we duplicate (some of) the current scsi b/w list, and we end up
filtering out commands that might be useful - like sending a MODE SENSE to
a disk for something other than cache information. (Would a user ever want
to send a MODE SENSE to USB storage device?)

A per scsi_host bflags (checked during scan, and possibly in mainline scsi
code, and perhaps propogated to a new scsi_device bflags) might be better,
especially if we can set it dynamically (at boot and eventually via driver
model/sysfs), and override it on a per SCSI device (vendor + product, have
the device info list have priority over scsi_host settings) basis.

Why can't usb storage conditionally modify the INQUIRY to request no more
than 36 bytes (allocation length), and modify any results to be <= 36
(additional length)?

Why isn't the BLIST_INQUIRY_36 or 58 flag used?

It looks like the SCSI_FILTER_INQUIRY_NOT36 might obsolete the dual
inquiry code in scsi_scan.c.

What happens to the scsi scan 2nd INQUIRY during scan - are we guaranteed
that the 1st INQUIRY never gets back a length over 36 from any user of the
SCSI_FILTER_INQUIRY_NOT36 filter? If not, we will fail the scan and not
find a device.


More generally:

It is not only USB scsi emulation that has problems. If we do not have a
dynamic solution (i.e. boot/module option or eventually something that can
be done at initrd time) we will (well, we do) have problems that require
code changes in order to avoid scsi errors. Some of the errors are
recoverable, but it is best to avoid the errors.

I don't see how a driver only code can solve these problems (if we want a
configurable flag or list), and given that we have more than just USB
storage having problems. A scsi_host bwflags might work, but AFAICT only
if it is always used (not just during scan, so we avoid problems with sg
usage).

devices below means scsi_device, adapter means the adapter driver or the
adapter driver hardware itself (for example megaraid card).

Besides the current b/w listed items (except for the BLIST_INQUIRY_36/58
flags), we have:

Devices (not adapter specific?) that hang on too long an INQUIRY request,
these can use BLIST_INQUIRY_36 or BLIST_INQUIRY_58

Devices (not adapter specific) that report back wrong INQUIRY length, so
we can't do a short INQUIRY followed by a long INQUIRY matching the length
returned; these can use BLIST_INQUIRY_36 or BLIST_INQUIRY_58

Devices (only known failure has been megaraid) that hang on REPORT LUNS

Devices (not adapter specific) that don't like EVPD INQUIRY

Devices (usb adapter specific and AFAIR actual devices) that don't like
MODE SENSE caching page (page code 0x8) (the SYNCHRONIZE_CACHE might have
also have problems).

-- Patrick Mansfield

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-04-07 at 17:33, Patrick Mansfield wrote:
> On Sat, Apr 05, 2003 at 09:30:41AM -0600, James Bottomley wrote:
> > On Mon, 2003-03-24 at 11:30, Matthew Dharm wrote:
> 
> James/Matt -
> 
> Some specific questions:
> 
> Why not just per command filtering? So either the adapter driver (or
> its underlying transport) supports a command or it does not. Other cases
> would need other b/w list support.

That's essentially what this filter is.

> Otherwise, we duplicate (some of) the current scsi b/w list, and we end up
> filtering out commands that might be useful - like sending a MODE SENSE to
> a disk for something other than cache information. (Would a user ever want
> to send a MODE SENSE to USB storage device?)

Actually, I'm trying to separate the two cases

1. Devices that have wierd problems, which the current b/w list copes
with and
2. HBA drivers that have command problems, primarily because they
emulate the scsi subsystem.

The filter is only for number 2.

> A per scsi_host bflags (checked during scan, and possibly in mainline scsi
> code, and perhaps propogated to a new scsi_device bflags) might be better,
> especially if we can set it dynamically (at boot and eventually via driver
> model/sysfs), and override it on a per SCSI device (vendor + product, have
> the device info list have priority over scsi_host settings) basis.

I like the filter approach because its not invasive to the current body
of the scsi code.

> Why can't usb storage conditionally modify the INQUIRY to request no more
> than 36 bytes (allocation length), and modify any results to be <= 36
> (additional length)?

I believe I asked that one at the time, but I don't remember getting a
crystal clear explanation.

> Why isn't the BLIST_INQUIRY_36 or 58 flag used?

in the filter?  I didn't want to reuse the BLIST_ flags as an opcode
because it could lead to confusion.

> It looks like the SCSI_FILTER_INQUIRY_NOT36 might obsolete the dual
> inquiry code in scsi_scan.c.
> 
> What happens to the scsi scan 2nd INQUIRY during scan - are we guaranteed
> that the 1st INQUIRY never gets back a length over 36 from any user of the
> SCSI_FILTER_INQUIRY_NOT36 filter? If not, we will fail the scan and not
> find a device.

I suppose that's a bug in scsi_scan.  Realistically, if we can get a 36
byte inquiry to the device, we should at least configure it regardless
of whether any subsequent inquiries fail.

> More generally:
> 
> It is not only USB scsi emulation that has problems. If we do not have a
> dynamic solution (i.e. boot/module option or eventually something that can
> be done at initrd time) we will (well, we do) have problems that require
> code changes in order to avoid scsi errors. Some of the errors are
> recoverable, but it is best to avoid the errors.

My preferred solution is to move large chunks of device configuration
into user space via hotplug.  However, we're not there yet...

> I don't see how a driver only code can solve these problems (if we want a
> configurable flag or list), and given that we have more than just USB
> storage having problems. A scsi_host bwflags might work, but AFAICT only
> if it is always used (not just during scan, so we avoid problems with sg
> usage).
> 
> devices below means scsi_device, adapter means the adapter driver or the
> adapter driver hardware itself (for example megaraid card).
> 
> Besides the current b/w listed items (except for the BLIST_INQUIRY_36/58
> flags), we have:
> 
> Devices (not adapter specific?) that hang on too long an INQUIRY request,
> these can use BLIST_INQUIRY_36 or BLIST_INQUIRY_58
> 
> Devices (not adapter specific) that report back wrong INQUIRY length, so
> we can't do a short INQUIRY followed by a long INQUIRY matching the length
> returned; these can use BLIST_INQUIRY_36 or BLIST_INQUIRY_58
> 
> Devices (only known failure has been megaraid) that hang on REPORT LUNS
> 
> Devices (not adapter specific) that don't like EVPD INQUIRY
> 
> Devices (usb adapter specific and AFAIR actual devices) that don't like
> MODE SENSE caching page (page code 0x8) (the SYNCHRONIZE_CACHE might have
> also have problems).

All the EVPD stuff should definitely be in user space regardless of
whether we move scanning out there.

James




-------------------------------------------------------
This SF.net email is sponsored by: ValueWeb: 
Dedicated Hosting for just $79/mo with 500 GB of bandwidth! 
No other company gives more support or power for your dedicated server
http://click.atdmt.com/AFF/go/sdnxxaff00300020aff/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Patrick Mansfield]

On Mon, Apr 07, 2003 at 06:14:28PM -0500, James Bottomley wrote:
> On Mon, 2003-04-07 at 17:33, Patrick Mansfield wrote:
> > On Sat, Apr 05, 2003 at 09:30:41AM -0600, James Bottomley wrote:
> > > On Mon, 2003-03-24 at 11:30, Matthew Dharm wrote:
> > 
> > James/Matt -
> > 
> > Some specific questions:
> > 
> > Why not just per command filtering? So either the adapter driver (or
> > its underlying transport) supports a command or it does not. Other cases
> > would need other b/w list support.
> 
> That's essentially what this filter is.

OK (I meant only filter on the command value and nothing else - no
scsi_filter_exceptions,  or a more generic scsi_filter_exceptions, but
given all the weird SCSI command flags, that is impractical.)

> > Otherwise, we duplicate (some of) the current scsi b/w list, and we end up
> > filtering out commands that might be useful - like sending a MODE SENSE to
> > a disk for something other than cache information. (Would a user ever want
> > to send a MODE SENSE to USB storage device?)
> 
> Actually, I'm trying to separate the two cases
> 
> 1. Devices that have wierd problems, which the current b/w list copes
> with and
> 2. HBA drivers that have command problems, primarily because they
> emulate the scsi subsystem.
> 
> The filter is only for number 2.
> 
> > A per scsi_host bflags (checked during scan, and possibly in mainline scsi
> > code, and perhaps propogated to a new scsi_device bflags) might be better,
> > especially if we can set it dynamically (at boot and eventually via driver
> > model/sysfs), and override it on a per SCSI device (vendor + product, have
> > the device info list have priority over scsi_host settings) basis.
> 
> I like the filter approach because its not invasive to the current body
> of the scsi code.

Yes, for any adapters or scsi devices that do not need it.

I don't see that it outweighs using a global (always) approach, with one
method to figure out if we can/cannot send the command, and one set of
flags or filters (though we might need two ways to set the flags, one per
hosts, and one per scsi_device).  

It won't help if a single device (versus adapter) has problems with any of
the new commands (cached, report luns, EVPD).

And, we probably need a common command line/boot option to add or modify
it (versus an occasional patch for each adapter that has problems, or a
patch to black list any of the new commands).

We could set a scsi_device bflags (or cmdflags or use your filter) only if
we have a scsi_device that needs special (command) handling, and end up
with mainline code (in scsi_request_fn or scsi_dispatch_cmd?) something
like:

	if (sdev->bflags && scsi_handle_bflags(sdev, scmd))
		return;

Where sdev->bflags (or a filter) is (generally) set at scan time based on
a b/w list and/or a scsi_host->bflags. If it's just an int it does limit
us to 32 (but if so we could go to a generic filter like your patch). I
would prefer a bit map now (probably simpler, especially for setting as a
boot option), and a filter if it is not enough.

It is an extra check (but not much compared to a lot of the current scsi
code called via scsi_dispatch_cmd or scsi_request_fn), and it would allow
the b/w list to be applied to all commands, not just those sent during
scanning (for example if we want to black list report luns or longer length
inquiries), and not on only a per-adapter driver basis.

> > Why isn't the BLIST_INQUIRY_36 or 58 flag used?
> 
> in the filter?  I didn't want to reuse the BLIST_ flags as an opcode
> because it could lead to confusion.

No - really why hasn't usb storage or anyone else used it?

> > More generally:
> > 
> > It is not only USB scsi emulation that has problems. If we do not have a
> > dynamic solution (i.e. boot/module option or eventually something that can
> > be done at initrd time) we will (well, we do) have problems that require
> > code changes in order to avoid scsi errors. Some of the errors are
> > recoverable, but it is best to avoid the errors.
> 
> My preferred solution is to move large chunks of device configuration
> into user space via hotplug.  However, we're not there yet...

Yes that would be quite nice to have.

> All the EVPD stuff should definitely be in user space regardless of
> whether we move scanning out there.

Can we just make it a config option for 2.5/6?

I'd like it to stay there for use with the multi-path patch, but that
patch could include (and hopefully white list) it.  (If we overflow the
number of allowed sd/block devices because there are multiple paths to
sd's, we can't come back in later and delete the extras, and then add in
ones that were rejected.)

Otherwise, I have to (sigh) agree it should ago. 

-- Patrick Mansfield


-------------------------------------------------------
This SF.net email is sponsored by: ValueWeb: 
Dedicated Hosting for just $79/mo with 500 GB of bandwidth! 
No other company gives more support or power for your dedicated server
http://click.atdmt.com/AFF/go/sdnxxaff00300020aff/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]

How about this patch?  It is based on James' work, but I cleaned up the
comments a bit and eliminated a structure which was used to only hold two
elements.  I also added a filter type and fixed the symbol problems when
modules were used.

I've tested this, and it works well.  Linus, if you'll take this I've got
several more patches -- ones to make usb-storage use this to cut some
undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
to be compatible with the filter code.

Linus, please apply this to 2.5.

Matt

# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.670   -> 1.671  
#	drivers/scsi/scsi_syms.c	1.20    -> 1.21   
#	 drivers/scsi/scsi.h	1.31    -> 1.32   
#	drivers/scsi/scsi_lib.c	1.34    -> 1.35   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/04/19	mdharm@zen.san.one-eyed-alien.net	1.671
# Added SCSI command filter.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/scsi.h b/drivers/scsi/scsi.h
--- a/drivers/scsi/scsi.h	Sat Apr 19 17:16:59 2003
+++ b/drivers/scsi/scsi.h	Sat Apr 19 17:16:59 2003
@@ -960,4 +960,52 @@
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 
+/* SCSI command filter
+ * 
+ * The 'command' array contains either (a) a SCSI command byte, or
+ * (b) an 'exception' rule.  These are distinguished by the high-byte;
+ * SCSI commands have 0 there, exceptions have some other value.
+ *
+ * Thus, *command < 0x100 is a SCSI command, >= 0x100 is an
+ * exception.  Exception codes are defined below.
+ *
+ * The format of the exceptions is:
+ *
+ * Bit 15: invert the specific condition
+ * Bit 14-12: Reserved
+ * Bit 11-8: command opcode (must be > 0)
+ * Bit 7-0: Command specific
+ */
+
+/* set filter type to whitelist or blacklist filtering */
+enum scsi_cmd_filter_type {
+	SCSI_FILTER_WHITELIST, SCSI_FILTER_BLACKLIST
+};
+
+extern int scsi_filter_cmd(struct scsi_cmnd *,
+		enum scsi_cmd_filter_type, unsigned short* );
+
+/* exception definitions for the filter */
+#define SCSI_FILTER_INVERT		0x8000
+
+/* opcodes for the filter */
+#define SCSI_FILTER_INQUIRY_LEN		0x0100
+#define SCSI_FILTER_INQUIRY_EVPD	0x0200
+
+/* marker for end of filter list */
+#define SCSI_FILTER_LIST_END		0xFFFF
+
+/* useful filter commands */
+#define SCSI_FILTER_INQUIRY_36		(SCSI_FILTER_INQUIRY_LEN | 36)
+#define SCSI_FILTER_INQUIRY_NOT36	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY_36)
+#define SCSI_FILTER_INQUIRY_NOT_EVPD	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY_EVPD)
+
+static inline unsigned short scsi_filter_opcode(unsigned short command) {
+	return command & 0x7f00;
+}
+
+static inline unsigned char scsi_filter_data(unsigned short command) {
+	return (unsigned char)(command & 0x00ff);
+}
+
 #endif /* _SCSI_H */
diff -Nru a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	Sat Apr 19 17:16:59 2003
+++ b/drivers/scsi/scsi_lib.c	Sat Apr 19 17:16:59 2003
@@ -1368,3 +1368,89 @@
 		kmem_cache_destroy(sgp->slab);
 	}
 }
+
+static inline int scsi_filter_exceptions(struct scsi_cmnd *cmd,
+					 unsigned short command)
+{
+	int found = 0;
+
+	/* specials begin at 0x100 */
+	if(command < 0x100)
+		return 0;
+
+	/* we check for inversion in each switch so that we can define each
+	 * test to act only on a specific subset of commands
+	 */
+	switch(scsi_filter_opcode(command)) {
+	case SCSI_FILTER_INQUIRY_EVPD:
+		if(cmd->cmnd[0] == INQUIRY) {
+			/* is EVPD bit set? */
+			found = (cmd->cmnd[1] & 0x1);
+
+			/* now check for inversion */
+			if(command & SCSI_FILTER_INVERT)
+				found = !found;
+		}
+		break;
+	case SCSI_FILTER_INQUIRY_LEN:
+		if(cmd->cmnd[0] == INQUIRY) {
+			/* does the transfer length match the data */
+			found = (cmd->cmnd[4] == scsi_filter_data(command));
+
+			/* now check for inversion */
+			if(command & SCSI_FILTER_INVERT)
+				found = !found;
+		}
+		break;
+	default:
+		/* unrecognized filter */
+		break;
+	}
+
+	return found;
+}
+
+/**
+ * scsi_filter_cmd - Filter a given command against a list
+ * @cmd: command to be filtered.
+ * @filter: pointer to the filter containing the type (black/white list) and
+ * zero terminated list of commands to filter against (first byte only).  See
+ * scsi.h for details on the filter structure.
+ *
+ * Returns 0 if the filter passed successfully and the driver can continue
+ * processing the command or 1 if the filter failed and the command should
+ * be finished (via ->scsi_done).  In the latter case, the command will have
+ * the sense fields filled in indicating the correct sense for an illegal
+ * request.
+ **/
+int scsi_filter_cmd(struct scsi_cmnd *cmd, enum scsi_cmd_filter_type type,
+		unsigned short* command)
+{
+	int found = 0;
+
+	/* search command list for a match */
+	for( ; *command != SCSI_FILTER_LIST_END; command++) {
+		found = scsi_filter_exceptions(cmd, *command);
+		found += (cmd->cmnd[0] == *command);
+		if(found)
+			break;
+	}
+
+	/* pass behavior -- found on a whitelist or not found on a blacklist */
+	if((found && type == SCSI_FILTER_WHITELIST) ||
+	   (!found && type == SCSI_FILTER_BLACKLIST))
+		return 0;
+
+	/* fill in the Check Condition/Illegal request */
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	memset(cmd->sense_buffer, '\0', sizeof(cmd->sense_buffer));
+	cmd->sense_buffer[0] = 0xF0; /* valid, response code 0x70 */
+	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
+
+	/* ASC 0x20, ASCQ 0x00: Invalid command operation code */
+	cmd->sense_buffer[12] = 0x20;
+	cmd->sense_buffer[13] = 0x00;
+
+	return 1;
+}
+	
diff -Nru a/drivers/scsi/scsi_syms.c b/drivers/scsi/scsi_syms.c
--- a/drivers/scsi/scsi_syms.c	Sat Apr 19 17:16:59 2003
+++ b/drivers/scsi/scsi_syms.c	Sat Apr 19 17:16:59 2003
@@ -55,6 +55,7 @@
 #if defined(CONFIG_SCSI_LOGGING)	/* { */
 EXPORT_SYMBOL(scsi_logging_level);
 #endif
+EXPORT_SYMBOL(scsi_filter_cmd);
 
 EXPORT_SYMBOL(scsi_allocate_request);
 EXPORT_SYMBOL(scsi_release_request);

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Would you mind not using our Web server? We're trying to have a game of 
Quake here.
					-- Greg
User Friendly, 5/11/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

On Sun, Apr 20, 2003 at 02:33:51PM -0700, Matthew Dharm wrote:
> I've tested this, and it works well.  Linus, if you'll take this I've got
> several more patches -- ones to make usb-storage use this to cut some
> undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
> to be compatible with the filter code.

Here is my patch to the SCSI scanning code.  Basically, if the first
INQUIRY for 36 bytes works, but a later one fails because of the filter, we
should still accept the device.

Linus, please apply.  I'll push the usb-storage patches via Greg K-H once
these are accepted.

Matt

# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.671   -> 1.672  
#	drivers/scsi/scsi_scan.c	1.28    -> 1.29   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/04/19	mdharm@zen.san.one-eyed-alien.net	1.672
# Modified probing routine so that HBAs which use the command filter to
# restrict INQUIRY to 36-bytes still get their devices detected.
# 
# This entire section of code may be bogus.  After all, if we got a good
# INQUIRY the first time, shouldn't we be on the guaranteed-accept path?
# --------------------------------------------
#
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Sat Apr 19 17:16:53 2003
+++ b/drivers/scsi/scsi_scan.c	Sat Apr 19 17:16:53 2003
@@ -1065,8 +1065,28 @@
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 2nd INQUIRY"
 				" %s with code 0x%x\n", sreq->sr_result ?
 				"failed" : "successful", sreq->sr_result));
-		if (sreq->sr_result)
-			return;
+
+		/* problem with the additional request */
+		if (sreq->sr_result) {
+			/* if it's anything but an ILLEGAL_REQUEST bail out */
+			if ((sreq->sr_sense_buffer[2] & 0xf) != ILLEGAL_REQUEST)
+				return;
+
+			/* a CHECK_CONDITION -- likely this came from the
+			 * command filter code.  We re-issue the 36-byte req
+			 * so we have good data to work with.
+			 */
+			possible_inq_resp_len = 36;
+			memset(scsi_cmd, 0, 6);
+			scsi_cmd[0] = INQUIRY;
+			scsi_cmd[4] = (unsigned char) possible_inq_resp_len;
+			sreq->sr_cmd_len = 0;
+			sreq->sr_data_direction = SCSI_DATA_READ;
+			memset(inq_result, 0, 36);
+			scsi_wait_req(sreq, (void *) scsi_cmd,
+					(void *) inq_result, 36,
+				       	SCSI_TIMEOUT + 4 * HZ, 3);
+		}
 
 		/*
 		 * The INQUIRY can change, this means the length can change.

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On Sun, 2003-04-20 at 16:35, Matthew Dharm wrote:
> On Sun, Apr 20, 2003 at 02:33:51PM -0700, Matthew Dharm wrote:
> > I've tested this, and it works well.  Linus, if you'll take this I've got
> > several more patches -- ones to make usb-storage use this to cut some
> > undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
> > to be compatible with the filter code.
> 
> Here is my patch to the SCSI scanning code.  Basically, if the first
> INQUIRY for 36 bytes works, but a later one fails because of the filter, we
> should still accept the device.
> 
> Linus, please apply.  I'll push the usb-storage patches via Greg K-H once
> these are accepted.

Actually, I'm not very keen about the levels of nested inquiry in this. 
How about the attached instead?

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 1141 bytes --]

===== drivers/scsi/scsi_scan.c 1.77 vs edited =====
--- 1.77/drivers/scsi/scsi_scan.c	Sun Apr 20 18:21:18 2003
+++ edited/drivers/scsi/scsi_scan.c	Mon Apr 21 11:17:20 2003
@@ -1001,6 +1001,7 @@
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	int possible_inq_resp_len;
 
+ repeat_inquiry:
 	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: INQUIRY to host %d"
 			" channel %d id %d lun %d\n", sdev->host->host_no,
 			sdev->channel, sdev->id, sdev->lun));
@@ -1067,8 +1068,14 @@
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 2nd INQUIRY"
 				" %s with code 0x%x\n", sreq->sr_result ?
 				"failed" : "successful", sreq->sr_result));
-		if (sreq->sr_result)
-			return;
+		if (sreq->sr_result) {
+			/* if the longer inquiry has failed, flag the device
+			 * as only accepting 36 byte inquiries and retry the
+			 * 36 byte inquiry */
+			printk(KERN_INFO "scsi scan: %d byte inquiry failed with code %d.  Consider BLIST_INQUIRY_36 for this device\n", sreq->sr_result);
+			*bflags |= BLIST_INQUIRY_36;
+			goto repeat_inquiry;
+		}
 
 		/*
 		 * The INQUIRY can change, this means the length can change.

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

I happen to think that most of this needs a rewrite into a well-structured
loop.  I'm not a big fan of goto.  But hey, it works and will probably get
us where we need to go without changing the behavior enough to disturb
other SCSI elements.

Matt

On Mon, Apr 21, 2003 at 11:20:40AM -0500, James Bottomley wrote:
> On Sun, 2003-04-20 at 16:35, Matthew Dharm wrote:
> > On Sun, Apr 20, 2003 at 02:33:51PM -0700, Matthew Dharm wrote:
> > > I've tested this, and it works well.  Linus, if you'll take this I've got
> > > several more patches -- ones to make usb-storage use this to cut some
> > > undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
> > > to be compatible with the filter code.
> > 
> > Here is my patch to the SCSI scanning code.  Basically, if the first
> > INQUIRY for 36 bytes works, but a later one fails because of the filter, we
> > should still accept the device.
> > 
> > Linus, please apply.  I'll push the usb-storage patches via Greg K-H once
> > these are accepted.
> 
> Actually, I'm not very keen about the levels of nested inquiry in this. 
> How about the attached instead?
> 
> James
> 

> ===== drivers/scsi/scsi_scan.c 1.77 vs edited =====
> --- 1.77/drivers/scsi/scsi_scan.c	Sun Apr 20 18:21:18 2003
> +++ edited/drivers/scsi/scsi_scan.c	Mon Apr 21 11:17:20 2003
> @@ -1001,6 +1001,7 @@
>  	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>  	int possible_inq_resp_len;
>  
> + repeat_inquiry:
>  	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: INQUIRY to host %d"
>  			" channel %d id %d lun %d\n", sdev->host->host_no,
>  			sdev->channel, sdev->id, sdev->lun));
> @@ -1067,8 +1068,14 @@
>  		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 2nd INQUIRY"
>  				" %s with code 0x%x\n", sreq->sr_result ?
>  				"failed" : "successful", sreq->sr_result));
> -		if (sreq->sr_result)
> -			return;
> +		if (sreq->sr_result) {
> +			/* if the longer inquiry has failed, flag the device
> +			 * as only accepting 36 byte inquiries and retry the
> +			 * 36 byte inquiry */
> +			printk(KERN_INFO "scsi scan: %d byte inquiry failed with code %d.  Consider BLIST_INQUIRY_36 for this device\n", sreq->sr_result);
> +			*bflags |= BLIST_INQUIRY_36;
> +			goto repeat_inquiry;
> +		}
>  
>  		/*
>  		 * The INQUIRY can change, this means the length can change.


-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Sun, 2003-04-20 at 16:33, Matthew Dharm wrote:
> How about this patch?  It is based on James' work, but I cleaned up the
> comments a bit and eliminated a structure which was used to only hold two
> elements.  I also added a filter type and fixed the symbol problems when
> modules were used.
> 
> I've tested this, and it works well.  Linus, if you'll take this I've got
> several more patches -- ones to make usb-storage use this to cut some
> undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
> to be compatible with the filter code.
> 
> Linus, please apply this to 2.5.

Actually, you havn't folded any of the list feedback into your version. 
I've done the merger and collapsed the EVPD inquiry filter into a
qualifier of the inquiry filter (which seems more appropriate).

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 7626 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1131  -> 1.1134 
#	 drivers/scsi/scsi.h	1.73    -> 1.76   
#	drivers/scsi/scsi_syms.c	1.30    -> 1.31   
#	drivers/scsi/scsi_lib.c	1.83    -> 1.86   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/04/21	jejb@raven.il.steeleye.com	1.1132
# Add a command filter to scsi_lib.c
# 
# To facilitate emulated hosts rejecting commands they do not
# wish to process
# --------------------------------------------
# 03/04/21	mdharm-scsi@one-eyed-alien.net	1.1131.1.1
# [PATCH] Re: PATCH: exclude certain commands from emulated SCSI hosts
# 
# --=-ubLHM6H8VjG+svJp0RLA
# Content-Disposition: inline
# Content-Type: text/plain; charset=us-ascii
# Content-Transfer-Encoding: 8bit
# 
# How about this patch?  It is based on James' work, but I cleaned up the
# comments a bit and eliminated a structure which was used to only hold two
# elements.  I also added a filter type and fixed the symbol problems when
# modules were used.
# 
# I've tested this, and it works well.  Linus, if you'll take this I've got
# several more patches -- ones to make usb-storage use this to cut some
# undesireable commands, and one to fix up the INQUIRY probing in scsi_scan.c
# to be compatible with the filter code.
# 
# Linus, please apply this to 2.5.
# 
# Matt
# 
# # This is a BitKeeper generated patch for the following project:
# # Project Name: greg k-h's linux 2.5 USB kernel tree
# # This patch format is intended for GNU patch command version 2.5 or higher.
# # This patch includes the following deltas:
# #	           ChangeSet	1.670   -> 1.671
# #	drivers/scsi/scsi_syms.c	1.20    -> 1.21
# #	 drivers/scsi/scsi.h	1.31    -> 1.32
# #	drivers/scsi/scsi_lib.c	1.34    -> 1.35
# #
# # The following is the BitKeeper ChangeSet Log
# # --------------------------------------------
# # 03/04/19	mdharm@zen.san.one-eyed-alien.net	1.671
# # Added SCSI command filter.
# # --------------------------------------------
# #
# --------------------------------------------
# 03/04/21	jejb@raven.il.steeleye.com	1.1133
# Merge jejb/mdharm
# --------------------------------------------
# 03/04/21	jejb@raven.il.steeleye.com	1.1134
# Complete jejb/mdharm Merger in scsi_filter_list
# --------------------------------------------
#
diff -Nru a/drivers/scsi/scsi.h b/drivers/scsi/scsi.h
--- a/drivers/scsi/scsi.h	Mon Apr 21 11:26:23 2003
+++ b/drivers/scsi/scsi.h	Mon Apr 21 11:26:23 2003
@@ -971,4 +971,49 @@
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 
+struct scsi_cmd_filter {
+	enum {
+		SCSI_FILTER_WHITELIST,
+		SCSI_FILTER_BLACKLIST
+	} type;
+	/* normal scsi commands are bytes, exceptions are longer.  The format
+	 * of the exceptions is:
+	 *
+	 * bit 15: invert the specific condition
+	 * bits 14-12: Reserved
+	 * bits 11-8: command opcode (must be > 0)
+	 * bits 7-0: Command specific */
+	unsigned short commands[];
+};
+
+extern int scsi_filter_cmd(struct scsi_cmnd *, struct scsi_cmd_filter *);
+
+/* exception definitions for the filter */
+#define SCSI_FILTER_INVERT		0x8000
+
+/* opcodes for the filter */
+#define SCSI_FILTER_INQUIRY		0x0100
+
+/* useful filter commands */
+
+/* of the FILTER_INQUIRY opcode, < 128 is a length bits above are inquiry
+ * type */
+#define SCSI_FILTER_QUALIFIER		0x80
+#define SCSI_FILTER_QUALIFIER_EVPD	0x01
+#define SCSI_FILTER_INQUIRY_EVPD	(SCSI_FILTER_INQUIRY | SCSI_FILTER_QUALIFIER | SCSI_FILTER_QUALIFIER_EVPD)
+#define SCSI_FILTER_INQUIRY_36		(SCSI_FILTER_INQUIRY | 36)
+#define SCSI_FILTER_INQUIRY_NOT36	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY | 36)
+#define SCSI_FILTER_INQUIRY_NOT_EVPD	(SCSI_FILTER_INVERT | SCSI_FILTER_INQUIRY_EVPD)
+
+/* marker for end of filter list */
+#define SCSI_FILTER_LIST_END		0xFFFF
+
+static inline unsigned short scsi_filter_opcode(unsigned short command) {
+	return command & 0x7f00;
+}
+
+static inline unsigned char scsi_filter_data(unsigned short command) {
+	return (unsigned char)(command & 0x00ff);
+}
+
 #endif /* _SCSI_H */
diff -Nru a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	Mon Apr 21 11:26:23 2003
+++ b/drivers/scsi/scsi_lib.c	Mon Apr 21 11:26:23 2003
@@ -1423,3 +1423,88 @@
 		kmem_cache_destroy(sgp->slab);
 	}
 }
+
+static inline int scsi_filter_exceptions(struct scsi_cmnd *cmd,
+					 unsigned short command)
+{
+	int found = 0;
+
+	/* specials begin at 0x100 */
+	if(command < 0x100)
+		return 0;
+
+	/* we check for inversion in each switch so that we can define each
+	 * test to act only on a specific subset of commands
+	 */
+	switch (scsi_filter_opcode(command)) {
+
+	case SCSI_FILTER_INQUIRY:
+		if (cmd->cmnd[0] != INQUIRY)
+			break;
+
+		/* is it a special filter */
+		if (command & SCSI_FILTER_QUALIFIER) {
+			if (command & SCSI_FILTER_QUALIFIER_EVPD)
+				/* is EVPD bit set? */
+				found += (cmd->cmnd[1] & 0x1) ? 1 : 0;
+		} else {
+			/* does the transfer length match the data */
+			found = (cmd->cmnd[4] == scsi_filter_data(command));
+		}
+		/* now check for inversion */
+		if (command & SCSI_FILTER_INVERT)
+			found = !found;
+		break;
+	default:
+		/* unrecognized filter */
+		break;
+	}
+
+	return found;
+}
+
+/**
+ * scsi_filter_cmd - Filter a given command against a list
+ * @cmd: command to be filtered.
+ * @filter: pointer to the filter containing the type (black/white list) and
+ * zero terminated list of commands to filter against (first byte only).  See
+ * scsi.h for details on the filter structure.
+ *
+ * Returns 0 if the filter passed successfully and the driver can continue
+ * processing the command or 1 if the filter failed and the command should
+ * be finished (via ->scsi_done).  In the latter case, the command will have
+ * the sense fields filled in indicating the correct sense for an illegal
+ * request.
+ **/
+int scsi_filter_cmd(struct scsi_cmnd *cmd, struct scsi_cmd_filter *filter)
+{
+	int found = 0;
+	unsigned short *command;
+
+	/* search command list for a match */
+	for (command = filter->commands ; *command != SCSI_FILTER_LIST_END;
+	     command++) {
+		found = scsi_filter_exceptions(cmd, *command);
+		found += (cmd->cmnd[0] == *command);
+		if(found)
+			break;
+	}
+
+	/* pass behavior -- found on a whitelist or not found on a blacklist */
+	if ((found && filter->type == SCSI_FILTER_WHITELIST) ||
+	    (!found && filter->type == SCSI_FILTER_BLACKLIST))
+		return 0;
+
+	/* fill in the Check Condition/Illegal request */
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	memset(cmd->sense_buffer, '\0', sizeof(cmd->sense_buffer));
+	cmd->sense_buffer[0] = 0xF0; /* valid, response code 0x70 */
+	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
+
+	/* ASC 0x20, ASCQ 0x00: Invalid command operation code */
+	cmd->sense_buffer[12] = 0x20;
+	cmd->sense_buffer[13] = 0x00;
+
+	return 1;
+}
+	
diff -Nru a/drivers/scsi/scsi_syms.c b/drivers/scsi/scsi_syms.c
--- a/drivers/scsi/scsi_syms.c	Mon Apr 21 11:26:23 2003
+++ b/drivers/scsi/scsi_syms.c	Mon Apr 21 11:26:23 2003
@@ -55,6 +55,7 @@
 #if defined(CONFIG_SCSI_LOGGING)	/* { */
 EXPORT_SYMBOL(scsi_logging_level);
 #endif
+EXPORT_SYMBOL(scsi_filter_cmd);
 
 EXPORT_SYMBOL(scsi_allocate_request);
 EXPORT_SYMBOL(scsi_release_request);

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

On Mon, Apr 21, 2003 at 11:28:48AM -0500, James Bottomley wrote:
> Actually, you havn't folded any of the list feedback into your version. 
> I've done the merger and collapsed the EVPD inquiry filter into a
> qualifier of the inquiry filter (which seems more appropriate).

I think 'any' is a bit harsh here... but, regardless...

My biggest complaints are:
(1) Your patch doesn't compile on my system.  RH7.3 -- it chokes on an
array in a struct with no length.  I doubt I'm the only person with this
problem.  Considering that in other places we bother to support this
compiler (-fomit-frame-pointer anyone?), it seems reasonable that we want
to support it here.

(2) You're creating a struct to associate two things (white/black selection
and the list) which don't really need to be associated.  A list is a list
-- if we treat it as whitelist or blacklist really is a separate question.

(3) Your treatment of the INQUIRY EVPD filter arbitrarily limits what the
filter can specify just to conserve a 'case' in a 'switch' -- considering
that the history of the problem starts with scsi_scan issuing INQUIRY for
255 bytes, making the filter unable to limit anything >=127 seems bad.

I stand by my original patch on this.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Why am I talking to a toilet brush?
					-- CEO
User Friendly, 4/30/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-04-21 at 12:01, Matthew Dharm wrote:
> I think 'any' is a bit harsh here... but, regardless...

OK, just the style changes, then, which are nasty to merge...

> My biggest complaints are:
> (1) Your patch doesn't compile on my system.  RH7.3 -- it chokes on an
> array in a struct with no length.  I doubt I'm the only person with this
> problem.  Considering that in other places we bother to support this
> compiler (-fomit-frame-pointer anyone?), it seems reasonable that we want
> to support it here.
>
> (2) You're creating a struct to associate two things (white/black selection
> and the list) which don't really need to be associated.  A list is a list
> -- if we treat it as whitelist or blacklist really is a separate question.

To be honest, I don't care much.  I think I had a merge problem where
the function template mismatched.

> (3) Your treatment of the INQUIRY EVPD filter arbitrarily limits what the
> filter can specify just to conserve a 'case' in a 'switch' -- considering
> that the history of the problem starts with scsi_scan issuing INQUIRY for
> 255 bytes, making the filter unable to limit anything >=127 seems bad.

I don't believe the limit to be a problem.  The scsi_scan code works
like this:

We send a 36 byte inquiry.  Based on the strings we get back we look for
devices that are blacklisted as supporting either 36 or 58 byte
inquiries.  If no blacklist exists, we believe the information the
device returns telling us how many bytes it supports for the inquiry, so
unless the device is blacklisted at 36, we issue another inquiry for
either 58 or the device listed length.

The blacklist only exists because you apparently have a device that lies
about the inquiry length it supports and then goes out to lunch when an
inquiry of that length is sent to it.

Therefore, the only current use of the inquiry filter is to black/white
list 36 and 58 byte inquiries.

Can you send us the inquiry strings of such a problem device?  We've had
absolutely no takers for the inquiry limitations in the current
blacklist, which does seem to imply that the code changes made to probe
all luns fixed the problem for almost everyone else.


James



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On Mon, Apr 21, 2003 at 02:23:36PM -0500, James Bottomley wrote:
> On Mon, 2003-04-21 at 12:01, Matthew Dharm wrote:
> > My biggest complaints are:
> > (1) Your patch doesn't compile on my system.  RH7.3 -- it chokes on an
> > array in a struct with no length.  I doubt I'm the only person with this
> > problem.  Considering that in other places we bother to support this
> > compiler (-fomit-frame-pointer anyone?), it seems reasonable that we want
> > to support it here.
> >
> > (2) You're creating a struct to associate two things (white/black selection
> > and the list) which don't really need to be associated.  A list is a list
> > -- if we treat it as whitelist or blacklist really is a separate question.
> 
> To be honest, I don't care much.  I think I had a merge problem where
> the function template mismatched.

Then can we have it the way I proposed?   I have tested patches that use
this code as I proposed it waiting to be merged as soon as this is
accepted.

> The blacklist only exists because you apparently have a device that lies
> about the inquiry length it supports and then goes out to lunch when an
> inquiry of that length is sent to it.
> 
> Therefore, the only current use of the inquiry filter is to black/white
> list 36 and 58 byte inquiries.
> 
> Can you send us the inquiry strings of such a problem device?  We've had
> absolutely no takers for the inquiry limitations in the current
> blacklist, which does seem to imply that the code changes made to probe
> all luns fixed the problem for almost everyone else.

I think takers may be waiting in the wings to see this code merged.
Honestly, tho, I don't think I can send you such strings.  I work pretty
closely with some vendors, so most of the devices in my possession have
'fixed' firmware (to report meaningful INQUIRY length).  However, I get
report after report of devices that blow this in new and creative ways from
end-users.

You may recall that my first approach was to set the BLIST flag for 36-byte
INQUIRY -- Linus shot that down in favor of a command-filter approach.

In the end, the limitation of what INQUIRY can be filtered is a minor
complaint compared to the others.  If I have to live with it, I will --
however I will suggest that instead of allowing a length < 127 to be
specified, why not just use a couple of bits to mean 36 and 58 -- i.e.
treat this like the EVPD logic.  If it's not going to be fully flexible,
then let's make it mean exactly what we want.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I see you've been reading alt.sex.chubby.sheep voraciously.
					-- Tanya
User Friendly, 11/24/97

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Mon, 2003-04-21 at 14:35, Matthew Dharm wrote:
> I think takers may be waiting in the wings to see this code merged.
> Honestly, tho, I don't think I can send you such strings.  I work pretty
> closely with some vendors, so most of the devices in my possession have
> 'fixed' firmware (to report meaningful INQUIRY length).  However, I get
> report after report of devices that blow this in new and creative ways from
> end-users.
> 
> You may recall that my first approach was to set the BLIST flag for 36-byte
> INQUIRY -- Linus shot that down in favor of a command-filter approach.

You mean we can junk the BLIST_INQUIRY flags?  Does the 58 byte inquiry
serve any purpose then (i.e. do you have a device that lies about its
inquiry length but can return 58 bytes?).  The 58 bytes is useful
because SPI fields lie in the 57th byte (information for the higher
ultra speeds), but on the other hand those bits are only defined for a
parallel bus.

If I junk the flags, we'll send a 36 byte inquiry and then one sized
from the inquiry return (unless we have to do an intermediate 58 byte
one) and let you filter them.  Does that sound OK?

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

James, I think we're misunderstanding each other here... I am in no way
advocating the removal of the BLIST_INQUIRY flag.

We've been through several variants of INQUIRY in the scanning code.
Nothing seems to make everyone happy.  My primary concern is that the SCSI
layer will send commands (a long INQUIRY being one of them) which will
crash the firmware of many devices.

All I want is a way to easily filter out those commands which devices on my
bus are likely (very likely) to choke over.  It's not just INQUIRY we're
talking about.

BLIST_INQUIRY serves some sort of alternate purpose.  It's never been
exactly clear to me what that is, but it seems to be related to firmware on
'real' SCSI devices being borked.

Besides, even if you fix this for INQUIRY, then we have to worry about
EVPD, MODE_SENSE for WP, 6-byte commands in general, etc. etc. etc.

Don't bother touching the flags.  Let's just settle on a filter
implementation.

Matt

On Mon, Apr 21, 2003 at 04:27:42PM -0500, James Bottomley wrote:
> On Mon, 2003-04-21 at 14:35, Matthew Dharm wrote:
> > I think takers may be waiting in the wings to see this code merged.
> > Honestly, tho, I don't think I can send you such strings.  I work pretty
> > closely with some vendors, so most of the devices in my possession have
> > 'fixed' firmware (to report meaningful INQUIRY length).  However, I get
> > report after report of devices that blow this in new and creative ways from
> > end-users.
> > 
> > You may recall that my first approach was to set the BLIST flag for 36-byte
> > INQUIRY -- Linus shot that down in favor of a command-filter approach.
> 
> You mean we can junk the BLIST_INQUIRY flags?  Does the 58 byte inquiry
> serve any purpose then (i.e. do you have a device that lies about its
> inquiry length but can return 58 bytes?).  The 58 bytes is useful
> because SPI fields lie in the 57th byte (information for the higher
> ultra speeds), but on the other hand those bits are only defined for a
> parallel bus.
> 
> If I junk the flags, we'll send a 36 byte inquiry and then one sized
> from the inquiry return (unless we have to do an intermediate 58 byte
> one) and let you filter them.  Does that sound OK?
> 
> James
> 

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

You were using cheat codes too.  You guys suck.
					-- Greg to General Studebaker
User Friendly, 12/16/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Patrick Mansfield]

Matthew -

On Mon, Apr 21, 2003 at 12:35:41PM -0700, Matthew Dharm wrote:

> Then can we have it the way I proposed?   I have tested patches that use
> this code as I proposed it waiting to be merged as soon as this is
> accepted.

I think it is OK, but as argued before: I was hoping to see attributes and
code that can be used for all scsi_devices off a particular adapter/LLDD,
or a specific scsi_device. Then we would not need separate attributes (or
filters) for adapters/LLDD and for the scsi_devices (the current BFLAGS
like code accessed via the scsi_dev_info_list; though being able to filter
any command is certainly more powerful).

We can currently modify the scsi_dev_info_list at boot time, and via /proc
(and eventually it could be modifiable via sysfs, after we have an
appropriate place for sysfs scsi-core driver attributes).

There have been reports of scsi devices that do not handle EVPD, they will
have to use something other than a per-adapter filter to block the
commands - probably another BFLAG. And a similiar problem with REPORT LUNS
(though that was adapter specific).

It would be very usefull to allow modification or addition of the filter
via user land (or at boot/insmod time), eventually via sysfs.  Embedding
the filter call in the queuecommand prevents such additions for arbitrary
adapters (i.e. if the adapter does not call the filter, we can't add any
filtering).

And we would be better off avoiding only the cache page (mode sense page
0x8) without blocking all mode sense commands (I assume that is what
you'll do when using the filter) - so we don't block other pages that
might be used (via sg), and we don't block the mode sense commands used to
used to figure out if the device is read only (sd.c sd_read_write_protect_flag,
though I don't understand WTF the second page code 0 is supposed to
return).

> You may recall that my first approach was to set the BLIST flag for 36-byte
> INQUIRY -- Linus shot that down in favor of a command-filter approach.

I thought his main objection was using the per-adapter emulated bit to
determine how to handle all scsi_devices on the adapter.

-- Patrick Mansfield

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

On Mon, Apr 21, 2003 at 02:28:05PM -0700, Patrick Mansfield wrote:
> Matthew -
> 
> On Mon, Apr 21, 2003 at 12:35:41PM -0700, Matthew Dharm wrote:
> 
> > Then can we have it the way I proposed?   I have tested patches that use
> > this code as I proposed it waiting to be merged as soon as this is
> > accepted.
> 
> I think it is OK, but as argued before: I was hoping to see attributes and
> code that can be used for all scsi_devices off a particular adapter/LLDD,
> or a specific scsi_device. Then we would not need separate attributes (or
> filters) for adapters/LLDD and for the scsi_devices (the current BFLAGS
> like code accessed via the scsi_dev_info_list; though being able to filter
> any command is certainly more powerful).

I think this is a nice idea, but you're talking about filtering at an
entirely different level from what I'm talking about.  I'm filtering at the
LLDD level, you want to filter in the mid-layer before the command gets out
the door to the LLDD.

> It would be very usefull to allow modification or addition of the filter
> via user land (or at boot/insmod time), eventually via sysfs.  Embedding
> the filter call in the queuecommand prevents such additions for arbitrary
> adapters (i.e. if the adapter does not call the filter, we can't add any
> filtering).

I can see how this would all be useful.  This, of course, assumes that you
can enumerate the device successfully (thus creating a device node) before
needing to tweak the flags, which is exactly NOT the case I'm dealing with.

I won't argue that such an implementation wouldn't be useful.  But it's a
_much_ bigger project than we're talking about here.  And, frankly, I think
it's a project for another day.

> And we would be better off avoiding only the cache page (mode sense page
> 0x8) without blocking all mode sense commands (I assume that is what
> you'll do when using the filter) - so we don't block other pages that
> might be used (via sg), and we don't block the mode sense commands used to
> used to figure out if the device is read only (sd.c sd_read_write_protect_flag,
> though I don't understand WTF the second page code 0 is supposed to
> return).

Actually, my plan is to block all MODE_SENSE for TYPE_DISK devices.  I'm
going to have some code that will not apply the filters (any filters) if
the request comes from sg, but that's down the line.  It turns out that a
very large number of USB devices of TYPE_DISK do _not_ support MODE_SENSE
in any form.  Apparently, the 'popular' OSes don't use it.

> > You may recall that my first approach was to set the BLIST flag for 36-byte
> > INQUIRY -- Linus shot that down in favor of a command-filter approach.
> 
> I thought his main objection was using the per-adapter emulated bit to
> determine how to handle all scsi_devices on the adapter.

Yes, which led to a discussion of where and how the filtering should take
place, which led to SCSI core code for filtering which would be applied by
whoever/whatever wanted to.

I agree that some sort of super-flexible multi-layered filtering system
would be great.  It might even use some of the same filter calls that we're
talking about here -- but that's a project for another day.  For now, let's
solve the the problem which has been with us since 2.3.x and get a filter
framework that usb-storage can use into place.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It's not that hard.  No matter what the problem is, tell the customer 
to reinstall Windows.
					-- Nurse
User Friendly, 3/22/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Linus Torvalds]

On Sun, 23 Mar 2003, Matthew Dharm wrote:

> On Sun, Mar 23, 2003 at 07:26:09PM -0600, James Bottomley wrote:
> > The problem with this type of approach is that there's no unified list
> > of "known good" commands that actually let you operate a device.  The
> > SCSI and ATAPI standards have been gradually deprecating the commands
> > that SCSI-1 (and sometimes even SCSI-2) regard as mandatory
> > (READ_6/WRITE_6 springs to mind).
> 
> Actually, there is such a list.  It's the commands that the 'popular OS'
> uses, and I have a pretty good idea exactly what those are.  That's why my
> original approach was to just cut out the commands that fell outside that
> definition.

Now _this_ is actually a valid approach, but then I would suggest you do
what Andries Brouwer has done a few times: just make the Linux SCSI layer
not use those commands _at_all_ - and instead use the sequences of
commands that "that other OS" uses. That fixed a lot of the smartmedia 
problems I used to have - doing simple things like _not_ trying to spin up 
devices that reported themselves to be already active etc.

This isn't an "emulated vs native" thing, btw, it's a much more generic 
"don't use commands that haven't gotten much testing" approach, and should 
probably be done both on emulated devices _and_ on "real" SCSI devices.

After all, "real" devices have problems too - I bet a lot of our
historical SCSI black-lists have been due to exactly the same issues you
see on emulated USB, with devices just reacting badly to command sequences
that they didn't expect and that hadn't been tested by the manufacturer.

			Linus

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

Well, I've already done that -- notice that we don't use START_STOP
anymore...

EVPD INQUIRY, INQUIRY for bytes != 36, and MODE_SENSE are big offenders.

But, EVPD INQUIRY is something that's a problem.  Getting rid of it isn't
easy -- someone put it in there because they wanted/needed the data.

INQUIRY != 36 bytes has been discussed before.  Since low-level drivers
snoop the data past 36-bytes, we can't completely get rid of this -- tho I
think this really means that the LLDD need fixing.

MODE_SENSE is something that Linux uses to check for write-protect status.
I could chuck the entire thing, but then we would assume write-enabled for
all disk-like media.

Note that not all of these fall into the 'not-well tested' category.
Certainly, INQUIRY != 36 is something that we blacklist (for having a long
history of problems).  MODE_SENSE doesn't have any historical problems that
I know about.  EVPD certainly hasn't had much testing.... but can I just
get rid of it then?

Matt

On Sun, Mar 23, 2003 at 05:58:43PM -0800, Linus Torvalds wrote:
> 
> On Sun, 23 Mar 2003, Matthew Dharm wrote:
> 
> > On Sun, Mar 23, 2003 at 07:26:09PM -0600, James Bottomley wrote:
> > > The problem with this type of approach is that there's no unified list
> > > of "known good" commands that actually let you operate a device.  The
> > > SCSI and ATAPI standards have been gradually deprecating the commands
> > > that SCSI-1 (and sometimes even SCSI-2) regard as mandatory
> > > (READ_6/WRITE_6 springs to mind).
> > 
> > Actually, there is such a list.  It's the commands that the 'popular OS'
> > uses, and I have a pretty good idea exactly what those are.  That's why my
> > original approach was to just cut out the commands that fell outside that
> > definition.
> 
> Now _this_ is actually a valid approach, but then I would suggest you do
> what Andries Brouwer has done a few times: just make the Linux SCSI layer
> not use those commands _at_all_ - and instead use the sequences of
> commands that "that other OS" uses. That fixed a lot of the smartmedia 
> problems I used to have - doing simple things like _not_ trying to spin up 
> devices that reported themselves to be already active etc.
> 
> This isn't an "emulated vs native" thing, btw, it's a much more generic 
> "don't use commands that haven't gotten much testing" approach, and should 
> probably be done both on emulated devices _and_ on "real" SCSI devices.
> 
> After all, "real" devices have problems too - I bet a lot of our
> historical SCSI black-lists have been due to exactly the same issues you
> see on emulated USB, with devices just reacting badly to command sequences
> that they didn't expect and that hadn't been tested by the manufacturer.
> 
> 			Linus

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm seen in many forms.  Now open your mouth.  It's caffeine time.
					-- Cola Man to Greg
User Friendly, 10/28/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [usb-storage] Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Tue, 2003-04-22 at 12:22, Andries.Brouwer@cwi.nl wrote:
[...]

Actually, could this

> Apr  9 17:34:55 lemon: usb-storage: Invoking Mode Translation

Be the root cause?  It indicates that usb storage modified the command.

In 2.5.68 that's doing a translation from MODE SENSE(6) to MODE
SENSE(10) inside the driver.

But the odd thing in usb/storage/protocol.c is this (about line 354):

                case MODE_SENSE:
                case MODE_SELECT:
                        srb->cmd_len = 12;
                        srb->cmnd[11] = 0;
                        srb->cmnd[10] = 0;

Why is a 10 byte command given a command length of 12?

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Alan Stern]

On 22 Apr 2003, James Bottomley wrote:

> On Tue, 2003-04-22 at 12:22, Andries.Brouwer@cwi.nl wrote:
> [...]
> 
> Actually, could this
> 
> > Apr  9 17:34:55 lemon: usb-storage: Invoking Mode Translation
> 
> Be the root cause?  It indicates that usb storage modified the command.

No, most likely it's not the cause.

> In 2.5.68 that's doing a translation from MODE SENSE(6) to MODE
> SENSE(10) inside the driver.
> 
> But the odd thing in usb/storage/protocol.c is this (about line 354):
> 
>                 case MODE_SENSE:
>                 case MODE_SELECT:
>                         srb->cmd_len = 12;
>                         srb->cmnd[11] = 0;
>                         srb->cmnd[10] = 0;
> 
> Why is a 10 byte command given a command length of 12?

The USB protocol used by this device is presumably one that requires all
commands to be 0-padded to 12 bytes.

More interesting is this:

> Apr  9 17:34:55 lemon: usb-storage: Command MODE_SENSE (6 bytes)
> Apr  9 17:34:55 lemon: usb-storage:  1a 00 3f 00 04 00

<snip>

> Apr  9 17:34:55 lemon: usb-storage: usb_stor_bulk_transfer_buf(): xfer 4 bytes
> Apr  9 17:34:55 lemon: usb-storage: Status code -75; transferred 4/4

The error is -EOVERFLOW, which means that the device tried to send back 
more data than we were prepared to receive, i.e., more than 4 bytes.  That 
4 comes from the second-to-last byte of the MODE-SENSE command.  But there 
is a comment in drivers/storage/usb/protocol.c indicating that 8 is the 
minimum size for a MODE-SENSE (line 257).  That might or might not be 
accurate, or it might not apply to this sort of device.  But it is 
suggestive.

Alan Stern

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Andries.Brouwer]

James writes:

> Why is a 10 byte command given a command length of 12?
> Could this be the root cause?

Alan S. answers:

> No, most likely it's not the cause.

And I agree - here 2.4 and 2.5 do not differ.

Alan continues:

> The error is -EOVERFLOW, which means that the device tried to send back
> more data than we were prepared to receive, i.e., more than 4 bytes.

And again I agree.

That is a pity, because my 4 was arrived at by careful experimentation.
It may mean that there is no uniform length that works.
And no amount of filtering will work, unless we decide
not to use MODE_SENSE at all.

But let us first confirm the theory.

In drivers/scsi/sd.c there is a routine sd_do_mode_sense6().
The theory is that calling that with length 4 causes problems
for some devices. So, could the people with problems try to
add

	if (len < 8)
		len = 8;

directly after

static int
sd_do_mode_sense6(struct scsi_device *sdp, struct scsi_request *SRpnt,
                  int dbd, int modepage, unsigned char *buffer, int len) {
        unsigned char cmd[8];

?

(Maybe try with very large values. like 255 instead of 8, in case
8 does not help.)

Andries

Re: PATCH: exclude certain commands from emulated SCSI hosts

[James Bottomley]

On Tue, 2003-04-22 at 14:30, Andries.Brouwer@cwi.nl wrote:
> That is a pity, because my 4 was arrived at by careful experimentation.
> It may mean that there is no uniform length that works.
> And no amount of filtering will work, unless we decide
> not to use MODE_SENSE at all.

Well, actually, we could do the filter in the same place that converts
the MODE SENSE(6) to MODE SENSE(10) (drivers/usb/storage/protocol.c). 
If it just returns an error when len < 8 we will drop through to the 255
byte request quickly which, on the 2.4 evidence, should work.

> But let us first confirm the theory.

Agreed.

James

Re: PATCH: exclude certain commands from emulated SCSI hosts

[Andries.Brouwer]

> we will drop through to the 255 byte request quickly which,
> on the 2.4 evidence, should work.

Yes, on the devices of these three people.
But not on many other devices.

Andries