[PATCH] Check bio address after mapping through partitions.

[PATCH] Check bio address after mapping through partitions.

[NeilBrown]

This would be good for 2.6.19 and even 18.2, if it is seens acceptable.
raid0 at least (possibly other) can be made to Oops with a bad partition 
table and best fix seem to be to not let out-of-range request get down
to the device.

### Comments for Changeset

Partitions are not limited to live within a device.  So
we should range check after partition mapping.

Note that 'maxsector' was being used for two different things.  I have
split off the second usage into 'old_sector' so that maxsector can be
still be used for it's primary usage later in the function.

Cc: Jens Axboe <jens.axboe@oracle.com>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2006-10-31 11:43:33.000000000 +1100
+++ ./block/ll_rw_blk.c	2006-10-31 12:47:07.000000000 +1100
@@ -3007,6 +3007,7 @@ static inline void __generic_make_reques
 {
 	request_queue_t *q;
 	sector_t maxsector;
+	sector_t old_sector;
 	int ret, nr_sectors = bio_sectors(bio);
 	dev_t old_dev;
 
@@ -3035,7 +3036,7 @@ static inline void __generic_make_reques
 	 * NOTE: we don't repeat the blk_size check for each new device.
 	 * Stacking drivers are expected to know what they are doing.
 	 */
-	maxsector = -1;
+	old_sector = -1;
 	old_dev = 0;
 	do {
 		char b[BDEVNAME_SIZE];
@@ -3069,15 +3070,30 @@ end_io:
 		 */
 		blk_partition_remap(bio);
 
-		if (maxsector != -1)
+		if (old_sector != -1)
 			blk_add_trace_remap(q, bio, old_dev, bio->bi_sector, 
-					    maxsector);
+					    old_sector);
 
 		blk_add_trace_bio(q, bio, BLK_TA_QUEUE);
 
-		maxsector = bio->bi_sector;
+		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;
 
+		maxsector = bio->bi_bdev->bd_inode->i_size >> 9;
+		if (maxsector) {
+			sector_t sector = bio->bi_sector;
+
+			if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+				/*
+				 * This may well happen - partitions are not checked
+				 * to make sure they are within the size of the
+				 * whole device.
+				 */
+				handle_bad_sector(bio);
+				goto end_io;
+			}
+		}
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);
 }

[PATCH] Check bio address after mapping through partitions.

[NeilBrown]

This would be good for 2.6.19 and even 18.2, if it is seens acceptable.
raid0 at least (possibly other) can be made to Oops with a bad partition 
table and best fix seem to be to not let out-of-range request get down
to the device.

### Comments for Changeset

Partitions are not limited to live within a device.  So
we should range check after partition mapping.

Note that 'maxsector' was being used for two different things.  I have
split off the second usage into 'old_sector' so that maxsector can be
still be used for it's primary usage later in the function.

Cc: Jens Axboe <jens.axboe@oracle.com>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2006-10-31 11:43:33.000000000 +1100
+++ ./block/ll_rw_blk.c	2006-10-31 12:47:07.000000000 +1100
@@ -3007,6 +3007,7 @@ static inline void __generic_make_reques
 {
 	request_queue_t *q;
 	sector_t maxsector;
+	sector_t old_sector;
 	int ret, nr_sectors = bio_sectors(bio);
 	dev_t old_dev;
 
@@ -3035,7 +3036,7 @@ static inline void __generic_make_reques
 	 * NOTE: we don't repeat the blk_size check for each new device.
 	 * Stacking drivers are expected to know what they are doing.
 	 */
-	maxsector = -1;
+	old_sector = -1;
 	old_dev = 0;
 	do {
 		char b[BDEVNAME_SIZE];
@@ -3069,15 +3070,30 @@ end_io:
 		 */
 		blk_partition_remap(bio);
 
-		if (maxsector != -1)
+		if (old_sector != -1)
 			blk_add_trace_remap(q, bio, old_dev, bio->bi_sector, 
-					    maxsector);
+					    old_sector);
 
 		blk_add_trace_bio(q, bio, BLK_TA_QUEUE);
 
-		maxsector = bio->bi_sector;
+		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;
 
+		maxsector = bio->bi_bdev->bd_inode->i_size >> 9;
+		if (maxsector) {
+			sector_t sector = bio->bi_sector;
+
+			if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+				/*
+				 * This may well happen - partitions are not checked
+				 * to make sure they are within the size of the
+				 * whole device.
+				 */
+				handle_bad_sector(bio);
+				goto end_io;
+			}
+		}
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);
 }

Re: [PATCH] Check bio address after mapping through partitions.

[Jens Axboe]

On Tue, Oct 31 2006, NeilBrown wrote:
> This would be good for 2.6.19 and even 18.2, if it is seens acceptable.
> raid0 at least (possibly other) can be made to Oops with a bad partition 
> table and best fix seem to be to not let out-of-range request get down
> to the device.
> 
> ### Comments for Changeset
> 
> Partitions are not limited to live within a device.  So
> we should range check after partition mapping.
> 
> Note that 'maxsector' was being used for two different things.  I have
> split off the second usage into 'old_sector' so that maxsector can be
> still be used for it's primary usage later in the function.
> 
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Signed-off-by: Neil Brown <neilb@suse.de>

Code looks good to me, but for some reason your comment exceeds 80
chars. Can you please fix that up?

Acked-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Jens Axboe

Re: [stable] [PATCH] Check bio address after mapping through partitions.

[Chris Wright]

* Jens Axboe (jens.axboe@oracle.com) wrote:
> On Tue, Oct 31 2006, NeilBrown wrote:
> > This would be good for 2.6.19 and even 18.2, if it is seens acceptable.
> > raid0 at least (possibly other) can be made to Oops with a bad partition 
> > table and best fix seem to be to not let out-of-range request get down
> > to the device.
> > 
> > ### Comments for Changeset
> > 
> > Partitions are not limited to live within a device.  So
> > we should range check after partition mapping.
> > 
> > Note that 'maxsector' was being used for two different things.  I have
> > split off the second usage into 'old_sector' so that maxsector can be
> > still be used for it's primary usage later in the function.
> > 
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > Signed-off-by: Neil Brown <neilb@suse.de>
> 
> Code looks good to me, but for some reason your comment exceeds 80
> chars. Can you please fix that up?

Maybe just was copy, pasted and pushed one tab over from the same check
above the loop.  What about consolidating that one?

thanks,
-chris

--

From: Neil Brown <neilb@suse.de>

Partitions are not limited to live within a device.  So
we should range check after partition mapping.
                                                                                                                   
Note that 'maxsector' was being used for two different things.  I have
split off the second usage into 'old_sector' so that maxsector can be
still be used for it's primary usage later in the function.
                                                                                                                   
Acked-by: Jens Axboe <jens.axboe@oracle.com>
Signed-off-by: Neil Brown <neilb@suse.de>

Add check_bad_sector() to consolidate the checks a touch.

Signed-off-by: Chris Wright <chrisw@sous-sol.org>

---
(smth like this only compile tested extension to Neil's patch)

 block/ll_rw_blk.c |   51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -2971,6 +2971,26 @@ static void handle_bad_sector(struct bio
 	set_bit(BIO_EOF, &bio->bi_flags);
 }
 
+static int check_bad_sector(struct bio *bio)
+{
+	sector_t maxsector = bio->bi_bdev->bd_inode->i_size >> 9;
+	int nr_sectors = bio_sectors(bio);
+	if (maxsector) {
+		sector_t sector = bio->bi_sector;
+
+		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+			/*
+			 * This may well happen - the kernel calls bread()
+			 * without checking the size of the device, e.g., when
+			 * mounting a device.
+			 */
+			handle_bad_sector(bio);
+			return 1;
+		}
+	}
+	return 0;
+}
+
 /**
  * generic_make_request: hand a buffer to its device driver for I/O
  * @bio:  The bio describing the location in memory and on the device.
@@ -2998,26 +3018,14 @@ static void handle_bad_sector(struct bio
 void generic_make_request(struct bio *bio)
 {
 	request_queue_t *q;
-	sector_t maxsector;
-	int ret, nr_sectors = bio_sectors(bio);
+	sector_t old_sector;
+	int ret;
 	dev_t old_dev;
 
 	might_sleep();
 	/* Test device or partition size, when known. */
-	maxsector = bio->bi_bdev->bd_inode->i_size >> 9;
-	if (maxsector) {
-		sector_t sector = bio->bi_sector;
-
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
-			/*
-			 * This may well happen - the kernel calls bread()
-			 * without checking the size of the device, e.g., when
-			 * mounting a device.
-			 */
-			handle_bad_sector(bio);
-			goto end_io;
-		}
-	}
+	if (check_bad_sector(bio))
+		goto end_io;
 
 	/*
 	 * Resolve the mapping until finished. (drivers are
@@ -3027,7 +3035,7 @@ void generic_make_request(struct bio *bi
 	 * NOTE: we don't repeat the blk_size check for each new device.
 	 * Stacking drivers are expected to know what they are doing.
 	 */
-	maxsector = -1;
+	old_sector = -1;
 	old_dev = 0;
 	do {
 		char b[BDEVNAME_SIZE];
@@ -3061,15 +3069,18 @@ end_io:
 		 */
 		blk_partition_remap(bio);
 
-		if (maxsector != -1)
+		if (old_sector != -1)
 			blk_add_trace_remap(q, bio, old_dev, bio->bi_sector, 
-					    maxsector);
+					    old_sector);
 
 		blk_add_trace_bio(q, bio, BLK_TA_QUEUE);
 
-		maxsector = bio->bi_sector;
+		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;
 
+		if (check_bad_sector(bio))
+			goto end_io;
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);
 }