[PATCH] block: bio_check_eod() needs to consider partition

[PATCH] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

bio_check_eod() should check partiton size not the whole disk if
bio->bi_partno is non-zero.

Based on an earlier patch from Jiufei Xue.

Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and partitions index")
Reported-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 76 ++++++++++++++++++++++----------------------------------
 1 file changed, 30 insertions(+), 46 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3ba4326a63b5..ba07f970e011 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2009,7 +2009,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static void handle_bad_sector(struct bio *bio)
+static void handle_bad_sector(struct bio *bio, sector_t maxsector)
 {
 	char b[BDEVNAME_SIZE];
 
@@ -2017,7 +2017,7 @@ static void handle_bad_sector(struct bio *bio)
 	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
 			bio_devname(bio, b), bio->bi_opf,
 			(unsigned long long)bio_end_sector(bio),
-			(long long)get_capacity(bio->bi_disk));
+			(long long)maxsector);
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -2060,57 +2060,47 @@ static inline bool should_fail_request(struct hd_struct *part,
  */
 static inline int blk_partition_remap(struct bio *bio)
 {
-	struct hd_struct *p;
-	int ret = 0;
+	sector_t maxsector = get_capacity(bio->bi_disk);
+	int nr_sectors = bio_sectors(bio);
 
 	/*
 	 * Zone reset does not include bi_size so bio_sectors() is always 0.
 	 * Include a test for the reset op code and perform the remap if needed.
 	 */
-	if (!bio->bi_partno ||
-	    (!bio_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET))
+	if (!nr_sectors && bio_op(bio) != REQ_OP_ZONE_RESET)
 		return 0;
 
-	rcu_read_lock();
-	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
-	if (likely(p && !should_fail_request(p, bio->bi_iter.bi_size))) {
+	if (bio->bi_partno) {
+		struct hd_struct *p;
+
+		rcu_read_lock();
+		p = __disk_get_part(bio->bi_disk, bio->bi_partno);
+		if (unlikely(!p ||
+			     should_fail_request(p, bio->bi_iter.bi_size))) {
+			rcu_read_unlock();
+			pr_info("%s: failing request for partition %d\n",
+				__func__, bio->bi_partno);
+			return -EIO;
+		}
+
 		bio->bi_iter.bi_sector += p->start_sect;
 		bio->bi_partno = 0;
 		trace_block_bio_remap(bio->bi_disk->queue, bio, part_devt(p),
 				bio->bi_iter.bi_sector - p->start_sect);
-	} else {
-		printk("%s: fail for partition %d\n", __func__, bio->bi_partno);
-		ret = -EIO;
+		maxsector = part_nr_sects_read(p);
+		rcu_read_unlock();
 	}
-	rcu_read_unlock();
 
-	return ret;
-}
-
-/*
- * Check whether this bio extends beyond the end of the device.
- */
-static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
-{
-	sector_t maxsector;
-
-	if (!nr_sectors)
-		return 0;
-
-	/* Test device or partition size, when known. */
-	maxsector = get_capacity(bio->bi_disk);
-	if (maxsector) {
-		sector_t sector = bio->bi_iter.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;
-		}
+	/*
+	 * Check whether this bio extends beyond the end of the device or
+	 * partition.  This may well happen - the kernel calls bread() without
+	 * checking the size of the device, e.g., when mounting a file system.
+	 */
+	if (nr_sectors && maxsector &&
+	    (nr_sectors > maxsector ||
+	     bio->bi_iter.bi_sector > maxsector - nr_sectors)) {
+		handle_bad_sector(bio, maxsector);
+		return -EIO;
 	}
 
 	return 0;
@@ -2126,9 +2116,6 @@ generic_make_request_checks(struct bio *bio)
 
 	might_sleep();
 
-	if (bio_check_eod(bio, nr_sectors))
-		goto end_io;
-
 	q = bio->bi_disk->queue;
 	if (unlikely(!q)) {
 		printk(KERN_ERR
@@ -2152,9 +2139,6 @@ generic_make_request_checks(struct bio *bio)
 	if (blk_partition_remap(bio))
 		goto end_io;
 
-	if (bio_check_eod(bio, nr_sectors))
-		goto end_io;
-
 	/*
 	 * Filter flush bio's early so that make_request based
 	 * drivers without flush support don't have to worry
-- 
2.14.2

Re: [PATCH] block: bio_check_eod() needs to consider partition

[Jiufei Xue]

Hi Christoph,

On 2018/3/8 下午3:46, Christoph Hellwig wrote:
> bio_check_eod() should check partiton size not the whole disk if
> bio->bi_partno is non-zero.
> 
I think the check should be done twice if the bio->bi_partno is not zero,
one for the partition, and another for the whole disk after remap which
is add in the commit 5ddfe9691c91
("md: check bio address after mapping through partitions").

Thanks,
Jiufei

> Based on an earlier patch from Jiufei Xue.
> 
> Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and partitions index")
> Reported-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-core.c | 76 ++++++++++++++++++++++----------------------------------
>  1 file changed, 30 insertions(+), 46 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3ba4326a63b5..ba07f970e011 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2009,7 +2009,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
>  	return BLK_QC_T_NONE;
>  }
>  
> -static void handle_bad_sector(struct bio *bio)
> +static void handle_bad_sector(struct bio *bio, sector_t maxsector)
>  {
>  	char b[BDEVNAME_SIZE];
>  
> @@ -2017,7 +2017,7 @@ static void handle_bad_sector(struct bio *bio)
>  	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
>  			bio_devname(bio, b), bio->bi_opf,
>  			(unsigned long long)bio_end_sector(bio),
> -			(long long)get_capacity(bio->bi_disk));
> +			(long long)maxsector);
>  }
>  
>  #ifdef CONFIG_FAIL_MAKE_REQUEST
> @@ -2060,57 +2060,47 @@ static inline bool should_fail_request(struct hd_struct *part,
>   */
>  static inline int blk_partition_remap(struct bio *bio)
>  {
> -	struct hd_struct *p;
> -	int ret = 0;
> +	sector_t maxsector = get_capacity(bio->bi_disk);
> +	int nr_sectors = bio_sectors(bio);
>  
>  	/*
>  	 * Zone reset does not include bi_size so bio_sectors() is always 0.
>  	 * Include a test for the reset op code and perform the remap if needed.
>  	 */
> -	if (!bio->bi_partno ||
> -	    (!bio_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET))
> +	if (!nr_sectors && bio_op(bio) != REQ_OP_ZONE_RESET)
>  		return 0;
>  
> -	rcu_read_lock();
> -	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> -	if (likely(p && !should_fail_request(p, bio->bi_iter.bi_size))) {
> +	if (bio->bi_partno) {
> +		struct hd_struct *p;
> +
> +		rcu_read_lock();
> +		p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> +		if (unlikely(!p ||
> +			     should_fail_request(p, bio->bi_iter.bi_size))) {
> +			rcu_read_unlock();
> +			pr_info("%s: failing request for partition %d\n",
> +				__func__, bio->bi_partno);
> +			return -EIO;
> +		}
> +
>  		bio->bi_iter.bi_sector += p->start_sect;
>  		bio->bi_partno = 0;
>  		trace_block_bio_remap(bio->bi_disk->queue, bio, part_devt(p),
>  				bio->bi_iter.bi_sector - p->start_sect);
> -	} else {
> -		printk("%s: fail for partition %d\n", __func__, bio->bi_partno);
> -		ret = -EIO;
> +		maxsector = part_nr_sects_read(p);
> +		rcu_read_unlock();
>  	}
> -	rcu_read_unlock();
>  
> -	return ret;
> -}
> -
> -/*
> - * Check whether this bio extends beyond the end of the device.
> - */
> -static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
> -{
> -	sector_t maxsector;
> -
> -	if (!nr_sectors)
> -		return 0;
> -
> -	/* Test device or partition size, when known. */
> -	maxsector = get_capacity(bio->bi_disk);
> -	if (maxsector) {
> -		sector_t sector = bio->bi_iter.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;
> -		}
> +	/*
> +	 * Check whether this bio extends beyond the end of the device or
> +	 * partition.  This may well happen - the kernel calls bread() without
> +	 * checking the size of the device, e.g., when mounting a file system.
> +	 */
> +	if (nr_sectors && maxsector &&
> +	    (nr_sectors > maxsector ||
> +	     bio->bi_iter.bi_sector > maxsector - nr_sectors)) {
> +		handle_bad_sector(bio, maxsector);
> +		return -EIO;
>  	}
>  
>  	return 0;
> @@ -2126,9 +2116,6 @@ generic_make_request_checks(struct bio *bio)
>  
>  	might_sleep();
>  
> -	if (bio_check_eod(bio, nr_sectors))
> -		goto end_io;
> -
>  	q = bio->bi_disk->queue;
>  	if (unlikely(!q)) {
>  		printk(KERN_ERR
> @@ -2152,9 +2139,6 @@ generic_make_request_checks(struct bio *bio)
>  	if (blk_partition_remap(bio))
>  		goto end_io;
>  
> -	if (bio_check_eod(bio, nr_sectors))
> -		goto end_io;
> -
>  	/*
>  	 * Filter flush bio's early so that make_request based
>  	 * drivers without flush support don't have to worry
> 

Re: [PATCH] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

On Thu, Mar 08, 2018 at 04:17:19PM +0800, Jiufei Xue wrote:
> Hi Christoph,
> 
> On 2018/3/8 下午3:46, Christoph Hellwig wrote:
> > bio_check_eod() should check partiton size not the whole disk if
> > bio->bi_partno is non-zero.
> > 
> I think the check should be done twice if the bio->bi_partno is not zero,
> one for the partition, and another for the whole disk after remap which
> is add in the commit 5ddfe9691c91
> ("md: check bio address after mapping through partitions").

I disagree.  See my other mail - if it fits inside the partition the
partition rescan code already made sure it also fits inside the device.

Re: [PATCH] block: bio_check_eod() needs to consider partition

[Jens Axboe]

On Thu, Mar 08 2018, Christoph Hellwig wrote:
> bio_check_eod() should check partiton size not the whole disk if
> bio->bi_partno is non-zero.
> 
> Based on an earlier patch from Jiufei Xue.

This doesn't apply, what did you generate it against?

-- 
Jens Axboe

Re: [PATCH] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

On Thu, Mar 08, 2018 at 02:09:23PM -0700, Jens Axboe wrote:
> On Thu, Mar 08 2018, Christoph Hellwig wrote:
> > bio_check_eod() should check partiton size not the whole disk if
> > bio->bi_partno is non-zero.
> > 
> > Based on an earlier patch from Jiufei Xue.
> 
> This doesn't apply, what did you generate it against?

Linux 4.15.  I'll respin it against latests 4.16-rc.