On Mon, Jul 05 2010 at 9:45am -0400, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > On Mon, Jul 05 2010 at 12:00am -0400, > FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote: > > > This can be applied to the block's for-2.6.36. > > > > = > > From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> > > Subject: [PATCH] scsi: unify the error handling of the prep functions > > > > This unifies the error handling of the prep functions (and fix the > > leak of a page allocated for discard in the case of BLKPREP_KILL or > > BLK_PREP_DEFER). > > > > The error handling of the prep path is very messy. Some errors are > > handled in the prep functions while some are in scsi_prep_return(). > > > > Let's handle all the errors in scsi_prep_return(). > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> > > This patch addresses the discard page leak too (scsi_prep_error calls > scsi_unprep_request which calls blk_unprep_request). > > Acked-by: Mike Snitzer <snitzer@xxxxxxxxxx> Hi, Your unified error handling forces a BLKPREP_KILL or BLKPREP_DEFER return from scsi_setup_discard_cmnd to depend on prep cleanup even though BLKPREP_OK was not returned. And this works, but it runs counter to the rules James previously shared: http://lkml.org/lkml/2010/7/1/512 "The rules are pretty clear: Unprep is only called if the request gets prepped ... that means you have to return BLKPREP_OK. Defer or kill assume there's no teardown to do, so the allocation (if it took place) must be reversed before returning them." All this being said, I'm OK with your patch (as my ack implies) but I can see that it will take the request prep error handling in a direction James may prefer to avoid. Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later. = From: Mike Snitzer <snitzer@xxxxxxxxxx> Subject: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path Currently, a discard request's page will not get cleaned up in the scsi_setup_discard_cmnd error path. A scsi_setup_discard_cmnd return other than BLKPREP_OK will not cause a discard request to get completely cleaned up (scsi_prep_return will not set REQ_DONTPREP unless BLKPREP_OK was returned). This fix eliminates the leak while preserving the rule that: Unprep is only called if the request gets prepped (meaning a return BLKPREP_OK). Defer or kill assume there's no teardown to do, so any allocation must be reversed before returning them. Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> --- drivers/scsi/sd.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 0994ab6..08e08bd 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -468,6 +468,10 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq) blk_add_request_payload(rq, page, len); ret = scsi_setup_blk_pc_cmnd(sdp, rq); rq->buffer = page_address(page); + if (ret != BLKPREP_OK) { + __free_page(page); + rq->buffer = NULL; + } return ret; } @@ -485,8 +489,10 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq) static void sd_unprep_fn(struct request_queue *q, struct request *rq) { - if (rq->cmd_flags & REQ_DISCARD) + if (rq->cmd_flags & REQ_DISCARD) { __free_page(virt_to_page(rq->buffer)); + rq->buffer = NULL; + } } /** -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
