[PATCH] scsi_lib: Bug in completion of bidi commands

[PATCH] scsi_lib: Bug in completion of bidi commands

[Boaz Harrosh]

Because of the terrible structuring of scsi-bidi-commands
it breaks some of the life time rules of a scsi-command.
It is now not allowed to free up the block-request before
cleanup and partial deallocation of the scsi-command. (Which
is not so for none bidi commands)

The right fix to this problem would be to make bidi command
a first citizen by allocating a scsi_sdb pointer at scsi command
just like cmd->prot_sdb. The bidi sdb should be allocated/deallocated
as part of the get/put_command (Again like the prot_sdb) and the
current decoupling of scsi_cmnd and blk-request should be kept.

For now make sure scsi_release_buffers() is called before the
call to blk_end_request_all() which might cause the suicide of
the block requests. At best the leak of bidi buffers, at worse
a crash, as there is a race between the existence of the bidi_request
and the free of the associated bidi_sdb.

The reason this was never hit before is because only OSD has the potential
of doing asynchronous bidi commands. (So does bsg but it is never used)
And OSD clients just happen to do all their bidi commands synchronously, up
until recently.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5987da8..bc9a881 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -749,9 +749,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			 */
 			req->next_rq->resid_len = scsi_in(cmd)->resid;
 
+			scsi_release_buffers(cmd);
 			blk_end_request_all(req, 0);
 
-			scsi_release_buffers(cmd);
 			scsi_next_command(cmd);
 			return;
 		}
-- 
1.6.5.2

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Boaz Harrosh]

On 12/15/2009 05:25 PM, Boaz Harrosh wrote:
> 
> Because of the terrible structuring of scsi-bidi-commands
> it breaks some of the life time rules of a scsi-command.
> It is now not allowed to free up the block-request before
> cleanup and partial deallocation of the scsi-command. (Which
> is not so for none bidi commands)
> 
> The right fix to this problem would be to make bidi command
> a first citizen by allocating a scsi_sdb pointer at scsi command
> just like cmd->prot_sdb. The bidi sdb should be allocated/deallocated
> as part of the get/put_command (Again like the prot_sdb) and the
> current decoupling of scsi_cmnd and blk-request should be kept.
> 
> For now make sure scsi_release_buffers() is called before the
> call to blk_end_request_all() which might cause the suicide of
> the block requests. At best the leak of bidi buffers, at worse
> a crash, as there is a race between the existence of the bidi_request
> and the free of the associated bidi_sdb.
> 
> The reason this was never hit before is because only OSD has the potential
> of doing asynchronous bidi commands. (So does bsg but it is never used)
> And OSD clients just happen to do all their bidi commands synchronously, up
> until recently.
> 
> CC: Stable Tree <stable@kernel.org>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

James hi.

What about this BUG. It affects anybody doing bidi commands. The possibilities
are an sglist leak at best, and a crash at worse.

I understand this code needs cleanup, but first things first. Lets first fix the
bug, which should also go to stable. Then the cleanup can go to next merge window.

BTW: Should I attempt a cleanup on current code, or should I wait for Alan's Patch
to go in first?

Thanks
Boaz

> ---
>  drivers/scsi/scsi_lib.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5987da8..bc9a881 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -749,9 +749,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			 */
>  			req->next_rq->resid_len = scsi_in(cmd)->resid;
>  
> +			scsi_release_buffers(cmd);
>  			blk_end_request_all(req, 0);
>  
> -			scsi_release_buffers(cmd);
>  			scsi_next_command(cmd);
>  			return;
>  		}

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Alan Stern]

On Thu, 31 Dec 2009, Boaz Harrosh wrote:

> James hi.
> 
> What about this BUG. It affects anybody doing bidi commands. The possibilities
> are an sglist leak at best, and a crash at worse.
> 
> I understand this code needs cleanup, but first things first. Lets first fix the
> bug, which should also go to stable. Then the cleanup can go to next merge window.
> 
> BTW: Should I attempt a cleanup on current code, or should I wait for Alan's Patch
> to go in first?

What patch of mine are you referring to?  So far James has rejected all
the patches I have submitted recently.  I'm going to try again in the 
near future...

Alan Stern

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Boaz Harrosh]

On 01/01/2010 04:46 AM, Alan Stern wrote:
> On Thu, 31 Dec 2009, Boaz Harrosh wrote:
> 
>> James hi.
>>
>> What about this BUG. It affects anybody doing bidi commands. The possibilities
>> are an sglist leak at best, and a crash at worse.
>>
>> I understand this code needs cleanup, but first things first. Lets first fix the
>> bug, which should also go to stable. Then the cleanup can go to next merge window.
>>
>> BTW: Should I attempt a cleanup on current code, or should I wait for Alan's Patch
>> to go in first?
> 
> What patch of mine are you referring to?  So far James has rejected all
> the patches I have submitted recently.  I'm going to try again in the 
> near future...
> 

OK, that's my answer, I didn't know.

Would you want that I attempt that collapsing of scsi_end_request() into scsi_io_completion
and the cleanup that implies? (that's the patch I meant.)

> Alan Stern
> 

Thanks && Happy new decade
Boaz

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Alan Stern]

On Sun, 3 Jan 2010, Boaz Harrosh wrote:

> > What patch of mine are you referring to?  So far James has rejected all
> > the patches I have submitted recently.  I'm going to try again in the 
> > near future...
> > 
> 
> OK, that's my answer, I didn't know.
> 
> Would you want that I attempt that collapsing of scsi_end_request() into scsi_io_completion
> and the cleanup that implies? (that's the patch I meant.)

Okay, I don't mind if you would like to rewrite that patch.  The 
version I wrote didn't just move code from one subroutine to another; 
it also made a few semantic changes (the retry counter and the "error" 
argument to blk_end_request()).  You'll probably want to break it 
up into a few patches, where the first simply moves the code around and 
the later ones do more significant things.

As I recall, the most recent version of that patch is here:

	http://marc.info/?l=linux-scsi&m=123991011815404&w=2

Alan Stern

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Boaz Harrosh]

On 01/03/2010 06:55 PM, Alan Stern wrote:
> On Sun, 3 Jan 2010, Boaz Harrosh wrote:
> 
>>> What patch of mine are you referring to?  So far James has rejected all
>>> the patches I have submitted recently.  I'm going to try again in the 
>>> near future...
>>>
>>
>> OK, that's my answer, I didn't know.
>>
>> Would you want that I attempt that collapsing of scsi_end_request() into scsi_io_completion
>> and the cleanup that implies? (that's the patch I meant.)
> 
> Okay, I don't mind if you would like to rewrite that patch.  The 
> version I wrote didn't just move code from one subroutine to another; 
> it also made a few semantic changes (the retry counter and the "error" 
> argument to blk_end_request()).  You'll probably want to break it 
> up into a few patches, where the first simply moves the code around and 
> the later ones do more significant things.
> 

Hi Alan, thanks

I'll only do the former and I'll let you continue with the later. .I.E the
code rearrangement and cleanup. Then perhaps it would be easier for you to
enhance the code with the retries and error returns. I do not have the setup
that can test / demonstrate those fixes, I'd rather you did them.

> As I recall, the most recent version of that patch is here:
> 
> 	http://marc.info/?l=linux-scsi&m=123991011815404&w=2
> 

Thanks

> Alan Stern
> 

Boaz

Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands

[Boaz Harrosh]

On 12/15/2009 05:25 PM, Boaz Harrosh wrote:
> 
> Because of the terrible structuring of scsi-bidi-commands
> it breaks some of the life time rules of a scsi-command.
> It is now not allowed to free up the block-request before
> cleanup and partial deallocation of the scsi-command. (Which
> is not so for none bidi commands)
> 
> The right fix to this problem would be to make bidi command
> a first citizen by allocating a scsi_sdb pointer at scsi command
> just like cmd->prot_sdb. The bidi sdb should be allocated/deallocated
> as part of the get/put_command (Again like the prot_sdb) and the
> current decoupling of scsi_cmnd and blk-request should be kept.
> 
> For now make sure scsi_release_buffers() is called before the
> call to blk_end_request_all() which might cause the suicide of
> the block requests. At best the leak of bidi buffers, at worse
> a crash, as there is a race between the existence of the bidi_request
> and the free of the associated bidi_sdb.
> 
> The reason this was never hit before is because only OSD has the potential
> of doing asynchronous bidi commands. (So does bsg but it is never used)
> And OSD clients just happen to do all their bidi commands synchronously, up
> until recently.
> 
> CC: Stable Tree <stable@kernel.org>

James hi.

Have you had the chance on looking at this issue. It's a serious bug
dated back a long time.

Technically it is quite simple.

It used to be:
	blk_end_request_all(req, 0);
	scsi_release_buffers(cmd);

Now scsi_release_buffers tries to use cmd->req for inspecting the req->next
pointer, but req was just freed by blk_end_request_all()

Reversing the call to:
	scsi_release_buffers(cmd);
	blk_end_request_all(req, 0);

Is the right thing to do and does not have any side effects what's so ever.

Please put me out of my misery ;-)
Boaz

> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/scsi_lib.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5987da8..bc9a881 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -749,9 +749,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			 */
>  			req->next_rq->resid_len = scsi_in(cmd)->resid;
>  
> +			scsi_release_buffers(cmd);
>  			blk_end_request_all(req, 0);
>  
> -			scsi_release_buffers(cmd);
>  			scsi_next_command(cmd);
>  			return;
>  		}