[BUG] Yet another scsi_cmnd leak?

[BUG] Yet another scsi_cmnd leak?

[Alan Stern]

James:

This report is based on 2.6.14-rc2-git6.  The code in your scsi-misc-2.6 
git tree is somewhat different (and I don't know which is more current), 
but it still contains the same bug.


In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
allocate an sg table.  When this happens, the scsi_cmnd isn't released and 
the request is not marked DONTPREP.

Then when scsi_prep_fn is called again, the request may be killed for 
a number of reasons.  The code branches to the kill: label near the end of 
the routine, which returns BLKPREP_KILL.

Isn't it true that when this happens, the scsi_cmnd allocated during the
original prep will never be released?

It appears that scsi_prep_fn is undecided about whether or not the request
is allowed to have a scsi_cmnd already.  The jumps to kill: seem to assume
that it isn't, but the code for allocating a new scsi_cmnd tests for an
existing one first.

Alan Stern

Re: [BUG] Yet another scsi_cmnd leak?

[Mike Christie]

Alan Stern wrote:
> James:
> 
> This report is based on 2.6.14-rc2-git6.  The code in your scsi-misc-2.6 
> git tree is somewhat different (and I don't know which is more current), 
> but it still contains the same bug.
> 
> 
> In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
> allocate an sg table.  When this happens, the scsi_cmnd isn't released and 
> the request is not marked DONTPREP.
> 
> Then when scsi_prep_fn is called again, the request may be killed for 
> a number of reasons.  The code branches to the kill: label near the end of 
> the routine, which returns BLKPREP_KILL.
> 
> Isn't it true that when this happens, the scsi_cmnd allocated during the
> original prep will never be released?
> 
> It appears that scsi_prep_fn is undecided about whether or not the request
> is allowed to have a scsi_cmnd already.  The jumps to kill: seem to assume
> that it isn't, but the code for allocating a new scsi_cmnd tests for an
> existing one first.
> 

The gotos used to be just a return BLKPREP* and were added so I did not 
have to write DID_NO_CONNECT or unplug multiple times :) I think you are 
right and we need to further unwind what a previous prep had done 
becuase when we return with BLKPREP_KILL we only hear about this command 
again if it's request has a end_io function or waiting completion.

Re: [BUG] Yet another scsi_cmnd leak?

[Mike Christie]

Mike Christie wrote:
> Alan Stern wrote:
> 
>> James:
>>
>> This report is based on 2.6.14-rc2-git6.  The code in your 
>> scsi-misc-2.6 git tree is somewhat different (and I don't know which 
>> is more current), but it still contains the same bug.
>>
>>
>> In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
>> allocate an sg table.  When this happens, the scsi_cmnd isn't released 
>> and the request is not marked DONTPREP.
>>
>> Then when scsi_prep_fn is called again, the request may be killed for 
>> a number of reasons.  The code branches to the kill: label near the 
>> end of the routine, which returns BLKPREP_KILL.
>>
>> Isn't it true that when this happens, the scsi_cmnd allocated during the
>> original prep will never be released?
>>
>> It appears that scsi_prep_fn is undecided about whether or not the 
>> request
>> is allowed to have a scsi_cmnd already.  The jumps to kill: seem to 
>> assume
>> that it isn't, but the code for allocating a new scsi_cmnd tests for an
>> existing one first.
>>
> 
> The gotos used to be just a return BLKPREP* and were added so I did not 
> have to write DID_NO_CONNECT or unplug multiple times :) I think you are 
> right and we need to further unwind what a previous prep had done 
> becuase when we return with BLKPREP_KILL we only hear about this command 

I did not mean command as in struct scsi_cmnd command. I think only the 
scsi_request or REQ_BLOCK_PC code sets the end_io or waiting fields so I 
really meant request or scsi_request.

> again if it's request has a end_io function or waiting completion.
> 

Re: [BUG] Yet another scsi_cmnd leak?

[Alan Stern]

On Thu, 29 Sep 2005, Mike Christie wrote:

> Alan Stern wrote:
> > James:
> > 
> > This report is based on 2.6.14-rc2-git6.  The code in your scsi-misc-2.6 
> > git tree is somewhat different (and I don't know which is more current), 
> > but it still contains the same bug.
> > 
> > 
> > In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
> > allocate an sg table.  When this happens, the scsi_cmnd isn't released and 
> > the request is not marked DONTPREP.
> > 
> > Then when scsi_prep_fn is called again, the request may be killed for 
> > a number of reasons.  The code branches to the kill: label near the end of 
> > the routine, which returns BLKPREP_KILL.
> > 
> > Isn't it true that when this happens, the scsi_cmnd allocated during the
> > original prep will never be released?
> > 
> > It appears that scsi_prep_fn is undecided about whether or not the request
> > is allowed to have a scsi_cmnd already.  The jumps to kill: seem to assume
> > that it isn't, but the code for allocating a new scsi_cmnd tests for an
> > existing one first.
> > 
> 
> The gotos used to be just a return BLKPREP* and were added so I did not 
> have to write DID_NO_CONNECT or unplug multiple times :) I think you are 
> right and we need to further unwind what a previous prep had done 
> becuase when we return with BLKPREP_KILL we only hear about this command 
> again if it's request has a end_io function or waiting completion.

I'm still not very clear about the conditions under which a request on the
queue can be partially prepared -- for example, scsi_cmnd assigned but 
not the sg table.  For the normal submission pathways, it looks like this 
happens only when the sg allocation fails.  In those cases it wouldn't 
hurt to release the scsi_cmnd before deferring.  Or before returning 
BLKPREP_KILL.

But what about other pathways?  As long as the special scsi_request things 
exist, I don't know what should be done.  I saw you had submitted patches 
to get rid of them; how far has that progressed?

Alan Stern

Re: [BUG] Yet another scsi_cmnd leak?

[Mike Christie]

Alan Stern wrote:
> On Thu, 29 Sep 2005, Mike Christie wrote:
> 
> 
>>Alan Stern wrote:
>>
>>>James:
>>>
>>>This report is based on 2.6.14-rc2-git6.  The code in your scsi-misc-2.6 
>>>git tree is somewhat different (and I don't know which is more current), 
>>>but it still contains the same bug.
>>>
>>>
>>>In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
>>>allocate an sg table.  When this happens, the scsi_cmnd isn't released and 
>>>the request is not marked DONTPREP.
>>>
>>>Then when scsi_prep_fn is called again, the request may be killed for 
>>>a number of reasons.  The code branches to the kill: label near the end of 
>>>the routine, which returns BLKPREP_KILL.
>>>
>>>Isn't it true that when this happens, the scsi_cmnd allocated during the
>>>original prep will never be released?
>>>
>>>It appears that scsi_prep_fn is undecided about whether or not the request
>>>is allowed to have a scsi_cmnd already.  The jumps to kill: seem to assume
>>>that it isn't, but the code for allocating a new scsi_cmnd tests for an
>>>existing one first.
>>>
>>
>>The gotos used to be just a return BLKPREP* and were added so I did not 
>>have to write DID_NO_CONNECT or unplug multiple times :) I think you are 
>>right and we need to further unwind what a previous prep had done 
>>becuase when we return with BLKPREP_KILL we only hear about this command 
>>again if it's request has a end_io function or waiting completion.
> 
> 
> I'm still not very clear about the conditions under which a request on the
> queue can be partially prepared -- for example, scsi_cmnd assigned but 
> not the sg table.  For the normal submission pathways, it looks like this 
> happens only when the sg allocation fails.  In those cases it wouldn't 
> hurt to release the scsi_cmnd before deferring.  Or before returning 
> BLKPREP_KILL.
> 
> But what about other pathways?  As long as the special scsi_request things 
> exist, I don't know what should be done.  I saw you had submitted patches 
> to get rid of them; how far has that progressed?
> 

I am not done. I still have osst to convert and I think Doug found a bug 
I cannot reproduce. I was not sure if everyone was happy with the 
max_sectors and the SCSI_MAX_PHYS_SEGMENTS compile option either.

I think I need to change the interface too. Passing in a scatterlist is 
nice beucase we do not have to touch the ULDs much, but I think if we 
went Christoph's route and used a array of bio_bvecs it might be nicer. 
We would need a bio helper function that could build bios from bvecs 
though and I think maybe that should be done based on the bioset stuff. 
I am not sure though.

[PATCH] Fix leak of Scsi_Cmnds

[Alan Stern]

James:

When a request is deferred in scsi_init_io because the sg table could not
be allocated, the associated scsi_cmnd is not released and the request is
not marked with REQ_DONTPREP.  When the command is retried, if
scsi_prep_fn decides to kill it then the scsi_cmnd will never be released.

This patch (as573) changes scsi_init_io so that it calls scsi_put_command 
before deferring a request.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

It may still be too soon to apply this patch.  I don't know how it will 
interact with the old scsi_request mechanism (if that is still being 
used).

Also, at some stage you will want to delete the code paths in scsi_prep_fn 
that assign cmd = req->special, because unprepared requests will _never_ 
have an already-associated scsi_cmnd.



Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -97,7 +97,6 @@ int scsi_insert_special_req(struct scsi_
 }
 
 static void scsi_run_queue(struct request_queue *q);
-static void scsi_release_buffers(struct scsi_cmnd *cmd);
 
 /*
  * Function:	scsi_unprep_request()
@@ -1040,8 +1039,10 @@ static int scsi_init_io(struct scsi_cmnd
 	 * if sg table allocation fails, requeue request later.
 	 */
 	sgpnt = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
-	if (unlikely(!sgpnt))
+	if (unlikely(!sgpnt)) {
+		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
+	}
 
 	cmd->request_buffer = (char *) sgpnt;
 	cmd->request_bufflen = req->nr_sectors << 9;
@@ -1245,8 +1246,8 @@ static int scsi_prep_fn(struct request_q
 		 */
 		ret = scsi_init_io(cmd);
 		switch(ret) {
+			/* For BLKPREP_KILL/DEFER the cmd was released */
 		case BLKPREP_KILL:
-			/* BLKPREP_KILL return also releases the command */
 			goto kill;
 		case BLKPREP_DEFER:
 			goto defer;