Re: [example PATCH - not for applying] exclude certain commands

Re: [example PATCH - not for applying] exclude certain commands

[Andries.Brouwer]

> The sd.c patch *is* needed to make it work.

Thanks for confirming.

Now that we know the facts, it is easy to answer my question

: But usb_stor_ufi_command() already contained code to
: make the length 8. How can it be that my sd.c change
: improved things?

The answer is that the length occurs twice - as part of
the SCSI command, and as bufflen. The usb-storage code
that fudges the command in the hope of producing something
more agreeable to the device forgets to adapt the bufflen.

This ends, I think, the discussion of what went wrong.
We understand perfectly - I hope.

Now about fixing the code. We must do something in usb-storage
or sd.c or both. I would prefer if sr_bufflen denotes the
actually available length, instead of the length that
will be transferred. If it has that meaning, then the
fudger in usb-storage knows for sure that there is space.
And sd_do_mode_sense6 and family can store the right number.

Are there any objections against viewing sr_bufflen
as the amount of actually available buffer space
(and thus potentially different from the transferred amount)?

Andries


-------------------------------------------------------
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: [example PATCH - not for applying] exclude certain commands

[Matthew Dharm]

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

On Fri, Apr 25, 2003 at 02:43:25AM +0200, Andries.Brouwer@cwi.nl wrote:
> Are there any objections against viewing sr_bufflen
> as the amount of actually available buffer space
> (and thus potentially different from the transferred amount)?

YES!  I MOST DEFINITELY OBJECT!

If you look through the linux-scsi archives, you'll see I've discussed this
before.  Apparently 'real' SCSI busses don't have this problem, but
usb-storage needs to know (in advance) how much data is going to be
transferred by a given command.

We used to have a tremendous amount of code which analyized the command in
an attempt to 'deduce' the correct answer.  It was right most of the time,
and failed completely on any vendor-specific command.

If you think about it, it's not really possible to figure out how much data
is going to be transfered based on the command bytes alone.  You may need
to know information such a the block size of the device, the device type,
etc.

Originally, I proposed adding a field to indicate how much data is actually
supposed to be transferred.  People objected to adding (including, I think,
Linus), and the general consensus was that sr_bufflen should indicate the
amount of data to be transferred.  The originator of the command was
responsible for making certain that there was actually that much buffer
available.

Short answer: usb-storage needs to know what the expected transfer amount
is.  The agreed-upon solution was to make sr_bufflen the expected transfer
amount instead of the available buffer size.

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: [linux-usb-devel] Re: [example PATCH - not for applying] exclude certain commands

[Matthew Dharm]

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

On Thu, Apr 24, 2003 at 07:12:38PM -0700, Matthew Dharm wrote:
> On Fri, Apr 25, 2003 at 02:43:25AM +0200, Andries.Brouwer@cwi.nl wrote:
> > Are there any objections against viewing sr_bufflen
> > as the amount of actually available buffer space
> > (and thus potentially different from the transferred amount)?
> 
> YES!  I MOST DEFINITELY OBJECT!

Before anyone reads this the wrong way, I want to apologize for the
shouting.  It was brought on by a combination of high stress at work and
the feeling that I was going around for another turn on the merry-go-round.

That's not an excuse just an explanation.

But the point remains, changing the meaning of sr_bufflen will break
usb-storage.

Matt

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

I don't have a left mouse button.  I only have one mouse and it's on my right.
					-- Customer
User Friendly, 2/13/1999

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

Re: [linux-usb-devel] Re: [example PATCH - not for applying] exclude certain commands

[Mike Bursell]

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

Folks -

I wonder if I could now be taken off the distribution list for this
thread?  It's gone more technical than I feel the need to be involved
with at this stage.  I'm very happy to test any more patches, however -
please let me know if there's anything you'd like looked at.

Thanks again for the help,

Yours,

-Mike.

On Fri, 2003-04-25 at 05:34, Matthew Dharm wrote:
> On Thu, Apr 24, 2003 at 07:12:38PM -0700, Matthew Dharm wrote:
> > On Fri, Apr 25, 2003 at 02:43:25AM +0200, Andries.Brouwer@cwi.nl wrote:
> > > Are there any objections against viewing sr_bufflen
> > > as the amount of actually available buffer space
> > > (and thus potentially different from the transferred amount)?
> > 
> > YES!  I MOST DEFINITELY OBJECT!
> 
> Before anyone reads this the wrong way, I want to apologize for the
> shouting.  It was brought on by a combination of high stress at work and
> the feeling that I was going around for another turn on the merry-go-round.
> 
> That's not an excuse just an explanation.
> 
> But the point remains, changing the meaning of sr_bufflen will break
> usb-storage.
> 
> Matt
-- 
07050 144575 mike@hingston.demon.co.uk

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [example PATCH - not for applying] exclude certain commands

[Alan Stern]

On Fri, 25 Apr 2003 Andries.Brouwer@cwi.nl wrote:

> Now about fixing the code. We must do something in usb-storage
> or sd.c or both. I would prefer if sr_bufflen denotes the
> actually available length, instead of the length that
> will be transferred. If it has that meaning, then the
> fudger in usb-storage knows for sure that there is space.
> And sd_do_mode_sense6 and family can store the right number.

There is a pattern emerging here, and I would like to propose a more 
general strategy for this problem.

We have seen that low-level device drivers such as usb-storage need to 
filter/alter the commands they accept.  Such actions include:

	(1) Refusing to issue a command that is known (or very likely) to 
crash the device's firmware.

	(2) Synthesizing an artificial reply to a command which the device 
doesn't handle well but the SCSI code needs.

	(3) Changing one command to another (e.g., READ(6) -> READ(10)).

	(4) Changing the data length for a command to meet the device's 
restrictions.

There may be other cases as well that I haven't thought of.

To begin with, in my opinion the LLDD should do a minimum of filtering and 
altering.  Not only is it frequently difficult to know if such actions are 
valid or will be acceptable to the caller, there is also the principle 
that user-generated requests (via the scsi-generic interface) should be 
left totally alone -- after all, the user may be probing precisely to 
determine whether or not the device will accept a particular command.

(1) is obviously necessary.  In general, refusing to accept a command is 
better than changing it, IMO.

(2) should be minimized, but there are times when it is unavoidable (such 
as INQUIRY for various USB storage devices that don't support it).

(3) should be avoided completely.  If the device doesn't accept some form 
of a command, then that fact should be transmitted back to the caller.

(4) also should be avoided.

This puts the burden of dealing with partially-compliant devices on the 
SCSI midlayer.  The routines there should be prepared to issue multiple 
forms of a some commands, so that if one form doesn't work another form 
can be tried.  Of course, this is already being done for READ and WRITE.  
A similar strategy could be used for things like MODE-SENSE.  However, 
detailed per-device information should be kept small -- little more than 
"this device likes the 6-byte commands rather than the 10-byte commands".

The same idea goes for retrying commands using different data lengths.  
It would be especially nice if there was a good way for the LLDD to report
that the data length specified was too short (or too long) for the device
to accept.

The filtering done by the LLDD should be relatively simple, and removing
the complex translations that are currently being done would help.  
Whether the filtering relies on centralized library routines (such as Matt
Dharm recently proposed) or a more decentralized scheme doesn't need to be
settled now.  In fact, it can fairly easily be changed later, as usage
patterns dictate.

This approach puts the burden of the various tasks where they belong.  
The LLDD is responsible for keeping the device from crashing, which only
it has the knowledge to do.  The SCSI mid-layer is responsible for the
strategies needed to get a device to carry out a particular action, which
is a problem that extends to all sorts of SCSI devices and is not peculiar
to one particular type.

Alan Stern

Re: [example PATCH - not for applying] exclude certain commands

[Oliver Neukum]

> To begin with, in my opinion the LLDD should do a minimum of filtering and
> altering.  Not only is it frequently difficult to know if such actions are
> valid or will be acceptable to the caller, there is also the principle
> that user-generated requests (via the scsi-generic interface) should be
> left totally alone -- after all, the user may be probing precisely to
> determine whether or not the device will accept a particular command.

Seriously, why this principle? It makes user space drivers inferior in
some respects. After all, we usually require a special capability to
allow actions that can crash hardware.

> (1) is obviously necessary.  In general, refusing to accept a command is
> better than changing it, IMO.

Better it would not be issued in the first place.

[..]
> This puts the burden of dealing with partially-compliant devices on the
> SCSI midlayer.  The routines there should be prepared to issue multiple
> forms of a some commands, so that if one form doesn't work another form
> can be tried.  Of course, this is already being done for READ and WRITE.

Wouldn't it be better if drivers had attributes which tell the midlevel
which forms of the several commands are acceptable?

	Regards
		Oliver

Re: [example PATCH - not for applying] exclude certain commands

[Alan Stern]

On Fri, 25 Apr 2003, Oliver Neukum wrote:

> > To begin with, in my opinion the LLDD should do a minimum of filtering and
> > altering.  Not only is it frequently difficult to know if such actions are
> > valid or will be acceptable to the caller, there is also the principle
> > that user-generated requests (via the scsi-generic interface) should be
> > left totally alone -- after all, the user may be probing precisely to
> > determine whether or not the device will accept a particular command.
> 
> Seriously, why this principle? It makes user space drivers inferior in
> some respects. After all, we usually require a special capability to
> allow actions that can crash hardware.

Do you mean it makes user space drivers inferior to kernel drivers?  I 
don't see how.  The LLDD would treat them both the same, doing a minimum 
of modifications.

If you mean that it would make user space drivers inferior to the way they
work now, then I agree.  At least, they might be inferior in terms of
performance, since they would no longer have commands altered
automatically for their benefit.  On the other hand, they would be
superior in terms of receiving a more transparent channel directly to the
device.  For some people this is very important.

I think it won't really be a problem.  A user-space driver _ought_ to be
prepared to cope with the quirks and special requirements of a particular
device.

> > (1) is obviously necessary.  In general, refusing to accept a command is
> > better than changing it, IMO.
> 
> Better it would not be issued in the first place.

True enough.  I'm not so sure that's really feasible at this point.  A 
large part of the debate that started this thread had to do with finding 
an appropriate way to represent the necessary information, and no 
satisfactory resolution was reached.

> [..]
> > This puts the burden of dealing with partially-compliant devices on the
> > SCSI midlayer.  The routines there should be prepared to issue multiple
> > forms of a some commands, so that if one form doesn't work another form
> > can be tried.  Of course, this is already being done for READ and WRITE.
> 
> Wouldn't it be better if drivers had attributes which tell the midlevel
> which forms of the several commands are acceptable?

Yes it would.  I'm not sure that everybody can agree on a good way to do 
this beyond the most rudimentary aspects.

Alan Stern

Re: [example PATCH - not for applying] exclude certain commands

[Oliver Neukum]

> Do you mean it makes user space drivers inferior to kernel drivers?  I
> don't see how.  The LLDD would treat them both the same, doing a minimum
> of modifications.

You got to make up your mind :-)
So the LLDD should filter in both cases?

> If you mean that it would make user space drivers inferior to the way they
> work now, then I agree.  At least, they might be inferior in terms of
> performance, since they would no longer have commands altered
> automatically for their benefit.  On the other hand, they would be
> superior in terms of receiving a more transparent channel directly to the
> device.  For some people this is very important.

For the larger part it's vastly important that exactly that doesn't happen.
Eg. I want the user to have access to a device, but I don't want him to
be able to crash the device. It's a multiuser system and others might
want to use the device afterwards.
We usually require root level to get a transparent channel to hardware.

If filtering is marginal you need to use yet another demon to be checked for
buffer overflows, etc. ... .

> I think it won't really be a problem.  A user-space driver _ought_ to be
> prepared to cope with the quirks and special requirements of a particular
> device.

But can you depend on it? From an administration viewpoint you create a
hole for a DoS attack.

	Regards
		Oliver

Re: [example PATCH - not for applying] exclude certain commands

[Alan Stern]

On Sat, 26 Apr 2003, Oliver Neukum wrote:

> > Do you mean it makes user space drivers inferior to kernel drivers?  I
> > don't see how.  The LLDD would treat them both the same, doing a minimum
> > of modifications.
> 
> You got to make up your mind :-)
> So the LLDD should filter in both cases?

Maybe I wasn't clear enough.  I want the LLDD to do exactly the same thing
regardless of where the request originates from.  I want it to do a
minimum of filtering, really just filtering out commands that are known to
crash the device.  I want it to do essentially no alteration of commands,
just enough to work at all.

In fact, I would argue that usb-storage is already doing too much.  It 
should not be converting between the 6-byte and 10-byte forms for several 
commands.  The caller should be smart enough to handle that.

The changes I'm proposing aren't that large.  There's just a couple of
rules the SCSI code should follow:

	(o) Use the 10-byte command form by default.  If it doesn't work, 
try the 6-byte form, and if that works better then flag the device entry 
as requiring the 6-byte form.  The READ and WRITE commands are already 
handled this way.

The MODE-SENSE problem that Andries was addressing wouldn't exist if this
rule had been followed.  The code tried the 6-byte form first and then did
not fall back to the 10-byte form when that failed.

	(o) Request the smallest-size data transfer that includes the data 
needed.  This is kind of vague, but I mean something like what the INQUIRY 
code now does: start with a minimum-sized 36-byte transfer and from that 
determine how much more is needed and supported.  Maybe try to stick with 
sizes that are commonly used by other popular OS's.

	If these two rules were followed carefully, we could remove a lot
of excess code from usb-storage and things would work more reliably.  So
far as I can see, the commands that would be affected are REQUEST-SENSE,
MODE-SENSE, MODE_SELECT, and maybe INQUIRY.

> > If you mean that it would make user space drivers inferior to the way they
> > work now, then I agree.  At least, they might be inferior in terms of
> > performance, since they would no longer have commands altered
> > automatically for their benefit.  On the other hand, they would be
> > superior in terms of receiving a more transparent channel directly to the
> > device.  For some people this is very important.
> 
> For the larger part it's vastly important that exactly that doesn't happen.
> Eg. I want the user to have access to a device, but I don't want him to
> be able to crash the device. It's a multiuser system and others might
> want to use the device afterwards.
> We usually require root level to get a transparent channel to hardware.

Nothing I propose involves relaxing permissions checking for access to
devices.  If root privileges were required before they will still be
required.  In addition, I am also proposing that the LLDD continue to
filter out commands that are known to crash the device.

If a user program wants convenient access to a device with the kernel
doing all the appropriate conversions to make things work, then the
program should be a SCSI _client_ and use the sd or sr facilities.  A
user-space _driver_ naturally must accept more responsibility for making
things work okay.

> If filtering is marginal you need to use yet another demon to be checked for
> buffer overflows, etc. ... .

No more than any other daemon or device driver requires.

> > I think it won't really be a problem.  A user-space driver _ought_ to be
> > prepared to cope with the quirks and special requirements of a particular
> > device.
> 
> But can you depend on it? From an administration viewpoint you create a
> hole for a DoS attack.

That's true for any user-space driver.  Consider an X11 server, for
example.  Of course a buggy user-space USB-SCSI driver can cause problems,
just as a buggy X11 server can.  The same mechanisms that protect against
one can protect against the other.

Alan Stern

Re: [example PATCH - not for applying] exclude certain commands

[Matthew Dharm]

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

On Sat, Apr 26, 2003 at 05:29:55PM -0500, James Bottomley wrote:
> This looks fine to me.  Originally, sr_bufflen was exactly that (because
> all SCSI LLDs have to be prepared to transfer fewer than the specified
> number of bytes anyway, since that's required by the spec).  I believe
> only USB needs this to be set to the actual command length (and further
> require that we ensure that the device *will* transfer exactly
> this...that's to some extent the origin of the INQUIRY problems), so the
> change should be small.

This is a common misconception.  usb-storage does not need to know, in
advance, exactly how many bytes will transfer.  It does, however, need to
know exactly how many bytes are being requested for transfer.

There's a subtle difference there.  usb-storage can handle the case of
underrun or overrun properly.  But there are USB<->[SCSI|ATA|whatever]
bridges which can't handle a discrepancy between expected transfer length
and actual transfer length when no error was detected.

I can give examples, but it's confusing and not really relevant unless
someone wants to know.

There are also broken devices which are specifically broken with respect to
the INQUIRY command for length != 36, but that's a separate issue to a
certain degree.  These are really two separate problems.

Matt

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

YOU SEE!!?? It's like being born with only one nipple!
					-- Erwin
User Friendly, 10/19/1998

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

Re: [linux-usb-devel] Re: [example PATCH - not for applying] exclude certain commands

[Alan Stern]

On Sat, 26 Apr 2003, Matthew Dharm wrote:

> This is a common misconception.  usb-storage does not need to know, in
> advance, exactly how many bytes will transfer.  It does, however, need to
> know exactly how many bytes are being requested for transfer.
> 
> There's a subtle difference there.  usb-storage can handle the case of
> underrun or overrun properly.  But there are USB<->[SCSI|ATA|whatever]
> bridges which can't handle a discrepancy between expected transfer length
> and actual transfer length when no error was detected.

I wonder about this a little, for overruns.  It's certainly true that we
detect an error and report problems when a transfer is larger than
expected (unless the expected length is exactly a multiple of the USB
packet size).  But it gets reported as DID_ERROR, indicating a possibly
transient communications problem.

This is just what Andries saw.  The MODE-SENSE commands wanted 4 bytes but
the devices insisted on sending more.  These waere reported as errors
rather than command failures (CHECK-CONDITION and so on), so the
error-handler kicked in and the same commands were retried to no avail.
That's no way to handle a systematic problem.

It would have worked better if instead we treated -EOVERFLOW as a failure, 
not an error.  Or if we had known that the device wouldn't work properly 
with a 4-byte MODE-SENSE(6) transfer.  We could have returned sense data 
that would tell the SCSI layer to try a different form of the command.

What I'm not sure about is the practicality of trying to handle things
this way.  Under what conditions is it correct to treat -EOVERFLOW during
the data phase as a failure instead of an error?  And do we want to try
and track the little foibles of many different sorts of devices so that we
can reject commands they don't handle properly?  Yes, we already do this
to some extent, but how far do we want to carry it through?

Alan Stern

Re: [example PATCH - not for applying] exclude certain commands

[James Bottomley]

On Thu, 2003-04-24 at 15:14, Matthew Dharm wrote:
> Well, at this point, I guess I should just be happy that something is being
> done.  But, what I'm really afraid of, this coming right back to this
> point.
> 
> In the past, I modified the write-protect probing in 2.4.x to be more compatible
> with what usb-storage wanted.  But then someone changed it, and it broke.
> So we changed it again... and it was fine for a while.  Then someone
> changed it in 2.5.x, and it broke again.
> 
> I guess what's got me frustrated is that this is very fragile, tends to
> cause serious problems when it fails, often gets changed, and isn't really
> necessary -- after all, that's why the 'assume write-enabled' code path is
> there for when the request fails.
> 
> I've just been around and around this merry-go-round, and I'm trying to get
> off.  We can fix it now, but I am truly afraid that it's going to get
> broken again in 6 months.

To some extent, I'm afraid you can't get off:  you're stuck on here with
the rest of us.

I can't guarantee that changes to the SCSI subsystem won't break USB
devices.  However, what I can guarantee is that we'll work to fix the
problems as they arise.

> And, as for the need for filtering, I know we have such a need.  I have
> several devices on my desk which choke at INQUIRY EVPD -- so right now I
> filter it in the usb-storage driver.  I also have devices that report bogus
> INQUIRY data lengths, but (luckily) the sanity checks in the INQUIRY
> probing code generally catch those cases and save us.

I think the evpd code can be thrown out into user land (where the rules
governing how it's used can be much more flexible).  All we really use
it for is to get a unique name.  However, even WWN inquiries don't
always guarantee even that...

> I've seen devices that choke on START_STOP, unless that START_STOP is an
> eject command.  I fixed this by eliminating the only non-eject use of
> START_STOP in sd.c -- but how long is it before someone decides they need
> to use START_STOP for something?

sd.c uses START_STOP to spin up an inactive disc (but only if we get a
check condition/NOT READY). sr_ioctl.c uses it to eject or close a cd
tray.

> We may have fixed MODE_SENSE, but what about the other cases?

I think we just have to identify them and tackle them in the same way.

Do you have any other current bug reports you'd like to share with us?

James

Re: [linux-usb-devel] Re: [example PATCH - not for applying] exclude certain commands

[Alan Stern]

On 24 Apr 2003, James Bottomley wrote:

> On Thu, 2003-04-24 at 15:14, Matthew Dharm wrote:
> > Well, at this point, I guess I should just be happy that something is being
> > done.  But, what I'm really afraid of, this coming right back to this
> > point.
> > 
> > In the past, I modified the write-protect probing in 2.4.x to be more compatible
> > with what usb-storage wanted.  But then someone changed it, and it broke.
> > So we changed it again... and it was fine for a while.  Then someone
> > changed it in 2.5.x, and it broke again.
> > 
> > I guess what's got me frustrated is that this is very fragile, tends to
> > cause serious problems when it fails, often gets changed, and isn't really
> > necessary -- after all, that's why the 'assume write-enabled' code path is
> > there for when the request fails.
> > 
> > I've just been around and around this merry-go-round, and I'm trying to get
> > off.  We can fix it now, but I am truly afraid that it's going to get
> > broken again in 6 months.
> 
> To some extent, I'm afraid you can't get off:  you're stuck on here with
> the rest of us.
> 
> I can't guarantee that changes to the SCSI subsystem won't break USB
> devices.  However, what I can guarantee is that we'll work to fix the
> problems as they arise.

Maybe it would help, just a little bit, to create a file in 
Documentation/scsi explaining some of the design choices and why they were 
made.  It could include a list of device types and the sorts of commands 
they choke on.

That way, when someone does change things in a way that breaks USB 
devices, you can at least say "You should have read 
Documentation/scsi/design_rules first!"

Alan Stern