James Bottomley <[email protected]>
Bcc:
Subject: Re: [PATCH] sbp2: fix panic when ejecting an ipod
Reply-To:
In-Reply-To: <[email protected]>
On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
> sbp2: fix panic when ejecting an ipod
>
> Sbp2 did not catch some bogus transfer directions in requests from upper
> layers. Problem became apparent when iPods were to be ejected:
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> Debugging and original variant of the patch by Andrew de Quincey.
NAK. James has a patch to fix this in the SCSI layer, which is his
preference.
Cheers,
Jody
>
> Signed-off-by: Stefan Richter <[email protected]>
> Cc: Andrew de Quincey <[email protected]>
>
> ---
>
> Corresponding fix of the places where transfer direction is actually set and
> how to clean sbp2_create_command_orb() up after this fix is being discussed.
> A patch for SCSI mid and high level has been submitted.
> http://marc.theaimsgroup.com/?t=113400010000001
> Please apply the following patch to prevent kernel panics as the first step.
>
> drivers/ieee1394/sbp2.c | 16 ++++++----------
> 1 files changed, 6 insertions(+), 10 deletions(-)
>
> --- linux/drivers/ieee1394.orig/sbp2.c 2005-11-24 23:10:21.000000000 +0100
> +++ linux/drivers/ieee1394/sbp2.c 2005-12-10 11:57:41.000000000 +0100
> @@ -1784,6 +1784,12 @@ static int sbp2_create_command_orb(struc
> break;
> }
>
> + if (orb_direction != ORB_DIRECTION_NO_DATA_TRANSFER &&
> + scsi_request_bufflen == 0) {
> + SBP2_WARN("Enforcing transfer direction DMA_NONE");
> + orb_direction = ORB_DIRECTION_NO_DATA_TRANSFER;
> + }
> +
> /*
> * Set-up our pagetable stuff... unfortunately, this has become
> * messier than I'd like. Need to clean this up a bit. ;-)
> @@ -1900,16 +1906,6 @@ static int sbp2_create_command_orb(struc
> command_orb->misc |= ORB_SET_DATA_SIZE(scsi_request_bufflen);
> command_orb->misc |= ORB_SET_DIRECTION(orb_direction);
>
> - /*
> - * Sanity, in case our direction table is not
> - * up-to-date
> - */
> - if (!scsi_request_bufflen) {
> - command_orb->data_descriptor_hi = 0x0;
> - command_orb->data_descriptor_lo = 0x0;
> - command_orb->misc |= ORB_SET_DIRECTION(1);
> - }
> -
> } else {
> /*
> * Need to turn this into page tables, since the
>
--
Jody McIntyre wrote:
> On Sat, Dec 10, 2005 at 12:24:59PM +0100, Stefan Richter wrote:
>
>>sbp2: fix panic when ejecting an ipod
>>
>>Sbp2 did not catch some bogus transfer directions in requests from upper
>>layers. Problem became apparent when iPods were to be ejected:
>>http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
>>http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
>>Debugging and original variant of the patch by Andrew de Quincey.
>
>
> NAK. James has a patch to fix this in the SCSI layer, which is his
> preference.
The fix for SCSI mid/high layer is to send the proper transfer
direction. (BTW, note that there are _many_ places where the transfer
direction is generated, not just where Jeff's and James' patches correct
things. The code which generates transfer directions will keep changing.
Userspace can set transfer directions.)
The fix for sbp2 is to not cause a kernel panic should an improper
transfer direction be send. All SCSI low level drivers have to handle
the transfer direction one way or another. Most chose not to panic.
My patch is not the fix for the wrong direction. It is solely meant not
to crash the whole machine after a minor mistake. Also, my patch is not
meant to hide mistakes that occurred in higer levels --- and it doesn't
do so.
I absolutely agree with you et al that bugs must be fixed in the layer
where they occur. However I also strongly believe that a bug in one
layer should not trickle down two or more layers and increase the damage
up to a catastrophe --- *if this can be avoided by simple means*, i.e.
without bloat. (Note the diffstat. I am basically moving existing code
up in an if/else cascade. Also, another cleanup for
sbp2_create_command_orb will follow next week.)
Please apply.
>>Signed-off-by: Stefan Richter <[email protected]>
>>Cc: Andrew de Quincey <[email protected]>
>>
>>---
>>
>>Corresponding fix of the places where transfer direction is actually set and
>>how to clean sbp2_create_command_orb() up after this fix is being discussed.
>>A patch for SCSI mid and high level has been submitted.
>>http://marc.theaimsgroup.com/?t=113400010000001
>>Please apply the following patch to prevent kernel panics as the first step.
>>
>> drivers/ieee1394/sbp2.c | 16 ++++++----------
>> 1 files changed, 6 insertions(+), 10 deletions(-)
>>
>>--- linux/drivers/ieee1394.orig/sbp2.c 2005-11-24 23:10:21.000000000 +0100
>>+++ linux/drivers/ieee1394/sbp2.c 2005-12-10 11:57:41.000000000 +0100
>>@@ -1784,6 +1784,12 @@ static int sbp2_create_command_orb(struc
>> break;
>> }
>>
>>+ if (orb_direction != ORB_DIRECTION_NO_DATA_TRANSFER &&
>>+ scsi_request_bufflen == 0) {
>>+ SBP2_WARN("Enforcing transfer direction DMA_NONE");
>>+ orb_direction = ORB_DIRECTION_NO_DATA_TRANSFER;
>>+ }
>>+
>> /*
>> * Set-up our pagetable stuff... unfortunately, this has become
>> * messier than I'd like. Need to clean this up a bit. ;-)
>>@@ -1900,16 +1906,6 @@ static int sbp2_create_command_orb(struc
>> command_orb->misc |= ORB_SET_DATA_SIZE(scsi_request_bufflen);
>> command_orb->misc |= ORB_SET_DIRECTION(orb_direction);
>>
>>- /*
>>- * Sanity, in case our direction table is not
>>- * up-to-date
>>- */
>>- if (!scsi_request_bufflen) {
>>- command_orb->data_descriptor_hi = 0x0;
>>- command_orb->data_descriptor_lo = 0x0;
>>- command_orb->misc |= ORB_SET_DIRECTION(1);
>>- }
>>-
>> } else {
>> /*
>> * Need to turn this into page tables, since the
>>
>
>
--
Stefan Richter
-=====-=-=-= ==-- -=-=-
http://arcgraph.de/sr/