bsg: bidi bio map failure fix

PATCH In review Pete Wyckoff 5 participants 10 messages Feb 12, 2008 – Feb 18, 2008

Subsystems (3)

SCSI Block layer BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

Pete Wyckoff

author

2008-02-12 20:40 UTC #0 lore raw

PATCH 1/1 bsg: bidi bio map failure fix

If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().

Signed-off-by Pete Wyckoff <[email protected]>

---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

block/bsg.c +3 -1

index 3337125..bba7154 100644
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
 			goto out;
+		}
 	}
 
 	if (hdr->dout_xfer_len) {

Signature · 1 line

1.5.3.8

James Bottomley

maintainer

2008-02-12 21:12 UTC #1 lore raw

Quoted 16 lines · Pete Wyckoff

If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644

block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 299

Quoted 6 lines

block/bsg.c +2 -1

@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */

Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map takes a request gets a ref and fills in the bio. The unmap has to be called on the bio leaving you to clear the now removed bio reference manually.

James

Jens Axboe

2008-02-18 12:55 UTC #2 lore raw

Quoted 18 lines · James Bottomley

Quoted 17 lines · Pete Wyckoff
If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 

Quoted 10 lines

block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 299
Quoted 5 lines
block/bsg.c +2 -1
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map takes a request gets a ref and fills in the bio. The unmap has to be called on the bio leaving you to clear the now removed bio reference manually.

It is nasty, how about fixing that instead?

block/blk-map.c +1 -0

index 955d75c..bc5ce60 100644
@@ -143,6 +143,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 	return 0;
 unmap_rq:
 	blk_rq_unmap_user(bio);
+	rq->bio = NULL;
 	return ret;
 }
 EXPORT_SYMBOL(blk_rq_map_user);

Signature · 1 line

Jens Axboe

FUJITA Tomonori

2008-02-18 14:37 UTC #3 lore raw

Quoted 30 lines · On Mon, 18 Feb 2008 13:55:08 +0100 Jens Axboe <[email protected]> wrote:

Quoted 18 lines · James Bottomley
Quoted 17 lines · Pete Wyckoff
If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 
Quoted 10 lines
block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 299
Quoted 5 lines
block/bsg.c +2 -1
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map takes a request gets a ref and fills in the bio. The unmap has to be called on the bio leaving you to clear the now removed bio reference manually.

Quoted 1 line

It is nasty, how about fixing that instead?

Yeah, looks better for me though the blk_rq_map_user API is still a bit hacky, as James said.

James, Pete's patch is still in scsi-fixes, so how about dropping it and sending this patch via the block?

Quoted 20 lines

block/blk-map.c +1 -0

index 955d75c..bc5ce60 100644
@@ -143,6 +143,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq,
 	return 0;
 unmap_rq:
 	blk_rq_unmap_user(bio);
+	rq->bio = NULL;
 	return ret;
 }
 EXPORT_SYMBOL(blk_rq_map_user);

Signature · 7 lines

-- 
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jens Axboe

2008-02-18 14:46 UTC #4 lore raw

Quoted 33 lines · FUJITA Tomonori

Quoted 30 lines · On Mon, 18 Feb 2008 13:55:08 +0100 Jens Axboe <[email protected]> wrote:
Quoted 28 lines · James Bottomley
Quoted 16 lines · Pete Wyckoff
If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644
block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 299
Quoted 6 lines
block/bsg.c +2 -1
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map takes a request gets a ref and fills in the bio. The unmap has to be called on the bio leaving you to clear the now removed bio reference manually.

Quoted 4 lines

Quoted 1 line
It is nasty, how about fixing that instead?
Yeah, looks better for me though the blk_rq_map_user API is still a bit hacky, as James said.

Seems symmetric to me now, either we fail and everything is cleaned up, or return success. What remains?

Signature · 1 line

Jens Axboe

James Bottomley

maintainer

2008-02-18 15:09 UTC #5 lore raw

Quoted 2 lines · Jens Axboe

Seems symmetric to me now, either we fail and everything is cleaned up, or return success. What remains?

My main symmetry complaint was the API: The map takes a request, the unmap takes a bio.

James

FUJITA Tomonori

2008-02-18 15:25 UTC #6 lore raw

Quoted 6 lines · On Mon, 18 Feb 2008 09:09:07 -0600 James Bottomley…

Quoted 2 lines · Jens Axboe
Seems symmetric to me now, either we fail and everything is cleaned up, or return success. What remains?
My main symmetry complaint was the API: The map takes a request, the unmap takes a bio.

Yeah, it would be nice if we avoid such code:

blk_rq_map_user(q, rq, ...)
bio = rq->bio
blk_execute_rq(q, ...
blk_rq_unmap_user(bio);

I think that none of the users of blk_rq_map_user is interested in bio, the details of how kernel manage I/Os. At least, we can remove bio stuff in bsg if blk_rq_map_user and blk_rq_unmap_user take requests.

Jens Axboe

2008-02-18 17:25 UTC #7 lore raw

In reply to James Bottomley

Quoted 6 lines · James Bottomley

Quoted 2 lines · Jens Axboe
Seems symmetric to me now, either we fail and everything is cleaned up, or return success. What remains?
My main symmetry complaint was the API: The map takes a request, the unmap takes a bio.

Oh that, well that was done to make it easier for existing users. Feel free to send a patch changing that, however it was done that way on purpose at the time being.

Signature · 1 line

Jens Axboe

James Bottomley

maintainer

2008-02-18 14:54 UTC #8 lore raw

In reply to FUJITA Tomonori

Quoted 33 lines · FUJITA Tomonori

Quoted 30 lines · On Mon, 18 Feb 2008 13:55:08 +0100 Jens Axboe <[email protected]> wrote:
Quoted 28 lines · James Bottomley
Quoted 16 lines · Pete Wyckoff
If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644
block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 299
Quoted 6 lines
block/bsg.c +2 -1
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map takes a request gets a ref and fills in the bio. The unmap has to be called on the bio leaving you to clear the now removed bio reference manually.

Quoted 7 lines

Quoted 1 line
It is nasty, how about fixing that instead?
Yeah, looks better for me though the blk_rq_map_user API is still a bit hacky, as James said.
James, Pete's patch is still in scsi-fixes, so how about dropping it and sending this patch via the block?

Yes, sure.

James

FUJITA Tomonori

maintainer Acked-by on v1 1/1

2008-02-14 11:57 UTC #9 lore raw

In reply to Pete Wyckoff

Quoted 17 lines · On Tue, 12 Feb 2008 15:40:24 -0500 Pete Wyckoff <[email protected]> wrote:

If blk_rq_map_user requires more than one bio, and fails mapping somewhere after the first bio, it will return with rq->bio set to non-NULL, but it will have already unmapped the partial bio. The "out:" error exit section will see the non-null bio and try to unmap it again, triggering a mapcount bug via bad_page().
Signed-off-by Pete Wyckoff <[email protected]>
---
 block/bsg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
block/bsg.c +0 -0
index 3337125..bba7154 100644
@@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 

block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr) · line 304

Quoted 10 lines

block/bsg.c +3 -1

 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
-		if (ret)
+		if (ret) {
+			next_rq->bio = NULL;  /* do not unmap twice */
 			goto out;
+		}
 	}
 
 	if (hdr->dout_xfer_len) {

Thanks!

Acked-by FUJITA Tomonori <[email protected]>

James, please put this to the scsi-fixes tree.