[PATCH] scsi: unify the error handling of the prep functions

[PATCH] scsi: unify the error handling of the prep functions

[FUJITA Tomonori]

This can be applied to the block's for-2.6.36.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
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@lab.ntt.co.jp>
---
 drivers/scsi/scsi_lib.c |   26 +++++++++++++-------------
 drivers/scsi/sd.c       |    6 ++++--
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ee83619..f9b3f7b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1008,14 +1008,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 	}
 
 	return BLKPREP_OK ;
-
 err_exit:
-	scsi_release_buffers(cmd);
-	if (error == BLKPREP_KILL)
-		scsi_put_command(cmd);
-	else /* BLKPREP_DEFER */
-		scsi_unprep_request(cmd->request);
-
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);
@@ -1177,6 +1170,17 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 }
 EXPORT_SYMBOL(scsi_prep_state_check);
 
+static void scsi_prep_error(struct request *req)
+{
+	struct scsi_cmnd *cmd = req->special;
+
+	if (!cmd)
+		return;
+
+	scsi_release_buffers(cmd);
+	scsi_unprep_request(req);
+}
+
 int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 {
 	struct scsi_device *sdev = q->queuedata;
@@ -1185,14 +1189,10 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 	case BLKPREP_KILL:
 		req->errors = DID_NO_CONNECT << 16;
 		/* release the command and kill it */
-		if (req->special) {
-			struct scsi_cmnd *cmd = req->special;
-			scsi_release_buffers(cmd);
-			scsi_put_command(cmd);
-			req->special = NULL;
-		}
+		scsi_prep_error(req);
 		break;
 	case BLKPREP_DEFER:
+		scsi_prep_error(req);
 		/*
 		 * If we defer, the blk_peek_request() returns NULL, but the
 		 * queue must be restarted, so we plug here if no returning
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index aa6b48b..88ba0c5 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -473,8 +473,10 @@ static int scsi_setup_discard_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)
-		__free_page(virt_to_page(rq->buffer));
+	if (rq->cmd_flags & REQ_DISCARD) {
+		free_page((unsigned long)rq->buffer);
+		rq->buffer = NULL;
+	}
 }
 
 /**
-- 
1.6.5

Re: scsi: unify the error handling of the prep functions

[Mike Snitzer]

On Mon, Jul 05 2010 at 12:00am -0400,
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> This can be applied to the block's for-2.6.36.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 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@lab.ntt.co.jp>

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@redhat.com>

[PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

[Mike Snitzer]

On Mon, Jul 05 2010 at  9:45am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Jul 05 2010 at 12:00am -0400,
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > This can be applied to the block's for-2.6.36.
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > 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@lab.ntt.co.jp>
> 
> 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@redhat.com>

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@redhat.com>
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@redhat.com>
---
 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;
+	}
 }
 
 /**

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

[Christoph Hellwig]

> 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.

I much prefer this approach.

> Signed-off-by: Mike Snitzer <snitzer@redhat.com>


Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path

[Jens Axboe]

On 07/07/10 21.44, Christoph Hellwig wrote:
>> 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.
> 
> I much prefer this approach.

Same here, it's the correct layering. I'll queue this up, thanks.

-- 
Jens Axboe

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

[FUJITA Tomonori]

On Tue, 6 Jul 2010 09:59:58 -0400
Mike Snitzer <snitzer@redhat.com> wrote:

> 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.

This patch doesn't work. I hit kernel oops since now this patch frees
a discard page twice.

If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable),
scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn
frees a page. Then, scsi_setup_discard_cmnd frees the same page again.


I talked to James on irc, he prefers to strictly apply the rule that
we should unprep on only the requests that got prepped.

So we need to remove scsi_unprep_request in scsi_init_io error
path. The following patch solves all the issues (hopefully).

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] scsi: fix discard page leak

We leak a page allocated for discard on some error conditions
(e.g. scsi_prep_state_check returns BLKPREP_DEFER in
scsi_setup_blk_pc_cmnd).

We unprep on requests that weren't prepped in the error path of
scsi_init_io. It makes the error path to clean up scsi commands messy.

Let's strictly apply the rule that we can't unprep on a request that
wasn't prepped.

Calling just scsi_put_command() in the error path of scsi_init_io() is
enough. We don't set REQ_DONTPREP yet.

scsi_setup_discard_cmnd can safely free a page on the error case with
the above rule.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/scsi_lib.c |    7 ++-----
 drivers/scsi/sd.c       |   10 ++++++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ee83619..b8de389 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1011,11 +1011,8 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 
 err_exit:
 	scsi_release_buffers(cmd);
-	if (error == BLKPREP_KILL)
-		scsi_put_command(cmd);
-	else /* BLKPREP_DEFER */
-		scsi_unprep_request(cmd->request);
-
+	scsi_put_command(cmd);
+	cmd->request->special = NULL;
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 0994ab6..1d0c4b7 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)
-		__free_page(virt_to_page(rq->buffer));
+	if (rq->cmd_flags & REQ_DISCARD) {
+		free_page((unsigned long)rq->buffer);
+		rq->buffer = NULL;
+	}
 }
 
 /**
-- 
1.6.5

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

[Mike Snitzer]

On Thu, Jul 08 2010 at 12:17am -0400,
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 6 Jul 2010 09:59:58 -0400
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > 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.
> 
> This patch doesn't work. I hit kernel oops since now this patch frees
> a discard page twice.

I load tested my v2 patch quite a bit but didn't hit the oops.  Guess
you're just lucky ;)

> If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable),
> scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn
> frees a page. Then, scsi_setup_discard_cmnd frees the same page again.

OK, thanks for catching this.

> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] scsi: fix discard page leak
> 
> We leak a page allocated for discard on some error conditions
> (e.g. scsi_prep_state_check returns BLKPREP_DEFER in
> scsi_setup_blk_pc_cmnd).
> 
> We unprep on requests that weren't prepped in the error path of
> scsi_init_io. It makes the error path to clean up scsi commands messy.
> 
> Let's strictly apply the rule that we can't unprep on a request that
> wasn't prepped.
> 
> Calling just scsi_put_command() in the error path of scsi_init_io() is
> enough. We don't set REQ_DONTPREP yet.
> 
> scsi_setup_discard_cmnd can safely free a page on the error case with
> the above rule.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Jens, it'd be great if we could get this fix in now.

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path

[Jens Axboe]

On 2010-07-08 14:06, Mike Snitzer wrote:
> On Thu, Jul 08 2010 at 12:17am -0400,
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
>> On Tue, 6 Jul 2010 09:59:58 -0400
>> Mike Snitzer <snitzer@redhat.com> wrote:
>>
>>> 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.
>>
>> This patch doesn't work. I hit kernel oops since now this patch frees
>> a discard page twice.
> 
> I load tested my v2 patch quite a bit but didn't hit the oops.  Guess
> you're just lucky ;)
> 
>> If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable),
>> scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn
>> frees a page. Then, scsi_setup_discard_cmnd frees the same page again.
> 
> OK, thanks for catching this.
> 
>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>> Subject: [PATCH] scsi: fix discard page leak
>>
>> We leak a page allocated for discard on some error conditions
>> (e.g. scsi_prep_state_check returns BLKPREP_DEFER in
>> scsi_setup_blk_pc_cmnd).
>>
>> We unprep on requests that weren't prepped in the error path of
>> scsi_init_io. It makes the error path to clean up scsi commands messy.
>>
>> Let's strictly apply the rule that we can't unprep on a request that
>> wasn't prepped.
>>
>> Calling just scsi_put_command() in the error path of scsi_init_io() is
>> enough. We don't set REQ_DONTPREP yet.
>>
>> scsi_setup_discard_cmnd can safely free a page on the error case with
>> the above rule.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Acked-by: Mike Snitzer <snitzer@redhat.com>
> 
> Jens, it'd be great if we could get this fix in now.

I picked up Tomo's patch this morning.


-- 
Jens Axboe