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

[Bart Van Assche]

On Wed, 2018-03-14 at 14:03 +0100, h...@lst.de wrote:
> can you test the version below?

Hello Christoph,

The same VM that failed to boot with v2 of this patch boots fine with this
patch.

Thanks,

Bart.

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

[h...@lst.de]

Hi Bart,

can you test the version below?

---
>From a68a8518158e31d66a0dc4f4e795ca3ceb83752c Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Tue, 13 Mar 2018 09:27:30 +0100
Subject: block: bio_check_eod() needs to consider partition

bio_check_eod() should check partiton size not the whole disk if
bio->bi_partno is non-zero.  Does this by taking the call to bio_check_eod
into blk_partition_remap.

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 
Signed-off-by: Christoph Hellwig 
---
 block/blk-core.c | 93 
 1 file changed, 40 insertions(+), 53 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6d82c4f7fadd..47ee24611126 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2023,7 +2023,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];
 
@@ -2031,7 +2031,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
@@ -2092,68 +2092,59 @@ static noinline int should_fail_bio(struct bio *bio)
 }
 ALLOW_ERROR_INJECTION(should_fail_bio, ERRNO);
 
+/*
+ * 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.
+ */
+static inline int bio_check_eod(struct bio *bio, sector_t maxsector)
+{
+   unsigned int nr_sectors = bio_sectors(bio);
+
+   if (nr_sectors && maxsector &&
+   (nr_sectors > maxsector ||
+bio->bi_iter.bi_sector > maxsector - nr_sectors)) {
+   handle_bad_sector(bio, maxsector);
+   return -EIO;
+   }
+   return 0;
+}
+
 /*
  * Remap block n of partition p to block n+start(p) of the disk.
  */
 static inline int blk_partition_remap(struct bio *bio)
 {
struct hd_struct *p;
-   int ret = 0;
+   int ret = -EIO;
 
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) ||
-bio_check_ro(bio, p))) {
-   ret = -EIO;
+   if (unlikely(!p))
+   goto out;
+   if (unlikely(should_fail_request(p, bio->bi_iter.bi_size)))
+   goto out;
+   if (unlikely(bio_check_ro(bio, p)))
goto out;
-   }
 
/*
 * 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_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET)
-   goto out;
-
-   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);
-
+   if (bio_sectors(bio) || bio_op(bio) == REQ_OP_ZONE_RESET) {
+   if (bio_check_eod(bio, part_nr_sects_read(p)))
+   goto out;
+   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);
+   }
+   ret = 0;
 out:
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;
-   }
-   }
-
-   return 0;
-}
-
 static noinline_for_stack bool
 generic_make_request_checks(struct bio *bio)
 {
@@ 

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

[Jens Axboe]

On 3/13/18 9:25 AM, Bart Van Assche wrote:
> On Tue, 2018-03-13 at 07:49 -0700, Jens Axboe wrote:
>> On 3/13/18 2:18 AM, Christoph Hellwig wrote:
>>> bio_check_eod() should check partiton size not the whole disk if
>>> bio->bi_partno is non-zero.  Does this by taking the call to bio_check_eod
>>> into blk_partition_remap.
>>
>> Applied, thanks.
> 
> Hello Christoph and Jens,
> 
> I think that this patch introduces a severe regression: with this patch 
> applied
> a VM that I use for kernel testing doesn't boot anymore. If I revert this 
> patch
> that VM boots fine again. This is what I see on the serial console with this
> patch applied:
> 
> virtio_blk virtio4: [vda] 41943040 512-byte logical blocks (21.5 GB/20.0 GiB)
>  vda: vda1
> attempt to access beyond end of device
> vda: rw=0, want=41940872, limit=41938944
> attempt to access beyond end of device
> vda: rw=0, want=41940872, limit=41938944

I've killed the patch for now, thanks for the quick test, Bart.

-- 
Jens Axboe

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

[h...@lst.de]

On Tue, Mar 13, 2018 at 04:25:40PM +, Bart Van Assche wrote:
> On Tue, 2018-03-13 at 07:49 -0700, Jens Axboe wrote:
> > On 3/13/18 2:18 AM, Christoph Hellwig wrote:
> > > bio_check_eod() should check partiton size not the whole disk if
> > > bio->bi_partno is non-zero.  Does this by taking the call to bio_check_eod
> > > into blk_partition_remap.
> > 
> > Applied, thanks.
> 
> Hello Christoph and Jens,
> 
> I think that this patch introduces a severe regression: with this patch 
> applied
> a VM that I use for kernel testing doesn't boot anymore. If I revert this 
> patch
> that VM boots fine again. This is what I see on the serial console with this
> patch applied:

Ok, please revert this for now.  I'm off for today and will look into
it tomorrow.

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

[Bart Van Assche]

On Tue, 2018-03-13 at 07:49 -0700, Jens Axboe wrote:
> On 3/13/18 2:18 AM, Christoph Hellwig wrote:
> > bio_check_eod() should check partiton size not the whole disk if
> > bio->bi_partno is non-zero.  Does this by taking the call to bio_check_eod
> > into blk_partition_remap.
> 
> Applied, thanks.

Hello Christoph and Jens,

I think that this patch introduces a severe regression: with this patch applied
a VM that I use for kernel testing doesn't boot anymore. If I revert this patch
that VM boots fine again. This is what I see on the serial console with this
patch applied:

virtio_blk virtio4: [vda] 41943040 512-byte logical blocks (21.5 GB/20.0 GiB)
 vda: vda1
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
scsi host2: Virtio SCSI HBA
scsi 2:0:0:0: Direct-Access QEMU QEMU HARDDISK2.5+ PQ: 0 ANSI: 5
sr 0:0:1:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
cdrom: Uniform CD-ROM driver Revision: 3.20
sr 0:0:1:0: Attached scsi CD-ROM sr0
sg_inq (142) used greatest stack depth: 26912 bytes left
systemd-udevd[90]: could not create device: Invalid argument
systemd-udevd[110]: Inserted 'virtio_scsi'
systemd-udevd[90]: Validate module index
systemd-udevd[110]: passed device to netlink monitor 0x55642693eed0
systemd-udevd[90]: Check if link configuration needs reloading.
systemd-udevd[90]: passed 146 byte device to netlink monitor 0x55642692e7d0
systemd-udevd[123]: passed device to netlink monitor 0x556426980e80
systemd-udevd[90]: passed 176 byte device to netlink monitor 0x55642692e7d0
systemd-udevd[123]: passed device to netlink monitor 0x556426980e80
systemd-udevd[90]: passed 179 byte device to netlink monitor 0x55642692e7d0
sd 2:0:0:0: Power-on or device reset occurred
sd 2:0:0:0: [sda] 5242880 512-byte logical blocks: (2.68 GB/2.50 GiB)
sd 2:0:0:0: [sda] Write Protect is off
sd 2:0:0:0: [sda] Mode Sense: 63 00 00 08
sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support 
DPO or FUA
systemd-udevd (123) used greatest stack depth: 26576 bytes left
systemd-udevd (121) used greatest stack depth: 24928 bytes left
systemd-udevd (110) used greatest stack depth: 24848 bytes left
sd 2:0:0:0: [sda] Attached SCSI disk
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
systemd-udevd[90]: Validate module index
systemd-udevd[90]: Check if link configuration needs reloading.
systemd-udevd[209]: IMPORT '/usr/bin/sg_inq --export /dev/sda' 
/lib/udev/rules.d/55-scsi-sg3_id.rules:10
systemd-udevd[211]: starting '/usr/bin/sg_inq --export /dev/sda'
systemd-udevd[210]: IMPORT builtin 'path_id' 
/lib/udev/rules.d/60-persistent-storage.rules:66
systemd-udevd[210]: LINK 'disk/by-path/pci-:00:07.0' 
/lib/udev/rules.d/60-persistent-storage.rules:68
systemd-udevd[210]: LINK 'disk/by-path/virtio-pci-:00:07.0' 
/lib/udev/rules.d/60-persistent-storage.rules:72
systemd-udevd[209]: '/usr/bin/sg_inq --export /dev/sda'(out) 'SCSI_TPGS=0'
systemd-udevd[210]: IMPORT builtin 'blkid' 
/lib/udev/rules.d/60-persistent-storage.rules:83
systemd-udevd[209]: '/usr/bin/sg_inq --export /dev/sda'(out) 'SCSI_TYPE=disk'
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical block 5242352, async page read
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
attempt to access beyond end of device
vda: rw=0, want=41940872, limit=41938944
Buffer I/O error on dev vda1, logical 

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

[Jens Axboe]

On 3/13/18 2:18 AM, Christoph Hellwig wrote:
> bio_check_eod() should check partiton size not the whole disk if
> bio->bi_partno is non-zero.  Does this by taking the call to bio_check_eod
> into blk_partition_remap.

Applied, thanks.

-- 
Jens Axboe