[PATCH v2] block: Improve limiting the bio size

[PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

bio_max_size() may get called before device_add_disk() and hence needs to
check whether or not the block device pointer is NULL. Additionally, more
code needs to be modified than __bio_try_merge_page() to limit the bio size
to bio_max_size().

This patch prevents that bio_max_size() triggers the following kernel
crash during a SCSI LUN scan:

BUG: KASAN: null-ptr-deref in bio_add_hw_page+0xa6/0x310
Read of size 8 at addr 00000000000005a8 by task kworker/u16:0/7
Workqueue: events_unbound async_run_entry_fn
Call Trace:
 show_stack+0x52/0x58
 dump_stack+0x9d/0xcf
 kasan_report.cold+0x4b/0x50
 __asan_load8+0x69/0x90
 bio_add_hw_page+0xa6/0x310
 bio_add_pc_page+0xaa/0xe0
 bio_map_kern+0xdc/0x1a0
 blk_rq_map_kern+0xcd/0x2d0
 __scsi_execute+0x9a/0x290 [scsi_mod]
 scsi_probe_lun.constprop.0+0x17c/0x660 [scsi_mod]
 scsi_probe_and_add_lun+0x178/0x750 [scsi_mod]
 __scsi_add_device+0x18c/0x1a0 [scsi_mod]
 ata_scsi_scan_host+0xe5/0x260 [libata]
 async_port_probe+0x94/0xb0 [libata]
 async_run_entry_fn+0x7d/0x2d0
 process_one_work+0x582/0xac0
 worker_thread+0x8f/0x5a0
 kthread+0x222/0x250
 ret_from_fork+0x1f/0x30

This patch also fixes the following kernel warning:

WARNING: CPU: 1 PID: 15449 at block/bio.c:1034
__bio_iov_iter_get_pages+0x324/0x350
Call Trace:
 bio_iov_iter_get_pages+0x6c/0x360
 __blkdev_direct_IO_simple+0x291/0x580
 blkdev_direct_IO+0xb5/0xc0
 generic_file_direct_write+0x10d/0x290
 __generic_file_write_iter+0x120/0x290
 blkdev_write_iter+0x16e/0x280
 new_sync_write+0x268/0x380
 vfs_write+0x3e0/0x4f0
 ksys_write+0xd9/0x180
 __x64_sys_write+0x43/0x50
 do_syscall_64+0x32/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Changheun Lee <nanich.lee@samsung.com>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Fixes: 15050b63567c ("bio: limit bio max size")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/bio.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Changes compared to v1: included a fix for direct I/O.

diff --git a/block/bio.c b/block/bio.c
index d718c63b3533..221dc56ba22f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -257,9 +257,9 @@ EXPORT_SYMBOL(bio_init);
 
 unsigned int bio_max_size(struct bio *bio)
 {
-	struct request_queue *q = bio->bi_bdev->bd_disk->queue;
+	struct block_device *bdev = bio->bi_bdev;
 
-	return q->limits.bio_max_bytes;
+	return bdev ? bdev->bd_disk->queue->limits.bio_max_bytes : UINT_MAX;
 }
 
 /**
@@ -1002,6 +1002,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 {
 	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
 	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
+	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
 	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
 	struct page **pages = (struct page **)bv;
 	bool same_page = false;
@@ -1017,7 +1018,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
 	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
 
-	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
+	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
+				  &offset);
 	if (unlikely(size <= 0))
 		return size ? size : -EFAULT;
 

Re: [PATCH v2] block: Improve limiting the bio size

[Yi Zhang]

Tested-by: Yi Zhang <yi.zhang@redhat.com>

This fixed the boot issue yesterday I found with the latest
linux-block/for-next on my R640 server.
[    2.511716] BUG: kernel NULL pointer dereference, address: 0000000000000370
[    2.518677] #PF: supervisor read access in kernel mode
[    2.523817] #PF: error_code(0x0000) - not-present page
[    2.528956] PGD 0 P4D 0
[    2.531494] Oops: 0000 [#1] SMP NOPTI
[    2.535160] CPU: 3 PID: 202 Comm: kworker/u64:31 Tainted: G
 I       5.12.0-rc8.e62a826b235f+ #4
[    2.544632] Hardware name: Dell Inc. PowerEdge R640/06NR82, BIOS
2.10.0 11/12/2020
[    2.552199] Workqueue: events_unbound async_run_entry_fn
[    2.557510] RIP: 0010:bio_add_hw_page+0x4f/0x180
[    2.562131] Code: 87 f7 00 00 00 0f b7 46 60 45 89 c4 89 cd 49 89
d5 48 89 f3 49 89 fe 66 85 c0 75 5f 66 39 43 62 0f 86 d6 00 00 00 48
8b 53 08 <48> 8b 92 70 03 00 00 48 8b 52 50 8b 92 08 04 00 00 29 ea 39
53 28
[    2.580876] RSP: 0018:ffffbe3300bebba0 EFLAGS: 00010202
[    2.586103] RAX: 0000000000000000 RBX: ffff99d941442b40 RCX: 0000000000000024
[    2.593234] RDX: 0000000000000000 RSI: ffff99d941442b40 RDI: ffff99d9d02b4dd0
[    2.600366] RBP: 0000000000000024 R08: 0000000000000500 R09: 0000000000000200
[    2.600630] usb 1-14.1: new high-speed USB device number 3 using xhci_hcd
[    2.607498] R10: 0000000000000024 R11: ffff99d9414442cf R12: 0000000000000500
[    2.607499] R13: ffffe05744131400 R14: ffff99d9d02b4dd0 R15: 0000000000000024
[    2.607500] FS:  0000000000000000(0000) GS:ffff99e87fac0000(0000)
knlGS:0000000000000000
[    2.607501] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.642364] CR2: 0000000000000370 CR3: 0000000132654006 CR4: 00000000007706e0
[    2.649496] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.656629] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.663761] PKRU: 55555554
[    2.666473] Call Trace:
[    2.668928]  bio_add_pc_page+0x30/0x50
[    2.672679]  blk_rq_map_kern+0x135/0x3e0
[    2.676604]  __scsi_execute+0x222/0x260
[    2.680444]  scsi_probe_and_add_lun+0x181/0xde0
[    2.684978]  __scsi_scan_target+0xec/0x5a0
[    2.689077]  scsi_scan_channel+0x5a/0x80
[    2.689728] usb 1-14.1: New USB device found, idVendor=1604,
idProduct=10c0, bcdDevice= 0.00
[    2.693001]  scsi_scan_host_selected+0xdb/0x110
[    2.701435] usb 1-14.1: New USB device strings: Mfr=0, Product=0,
SerialNumber=0
[    2.705966]  do_scan_async+0x17/0x150
[    2.705969]  async_run_entry_fn+0x39/0x160
[    2.713895] hub 1-14.1:1.0: USB hub found
[    2.717025]  process_one_work+0x1cb/0x360
[    2.721183] hub 1-14.1:1.0: 4 ports detected
[    2.725136]  ? process_one_work+0x360/0x360
[    2.725137]  worker_thread+0x30/0x370
[    2.725139]  ? process_one_work+0x360/0x360
[    2.745433]  kthread+0x116/0x130
[    2.748666]  ? kthread_park+0x80/0x80
[    2.752331]  ret_from_fork+0x1f/0x30
[    2.755911] Modules linked in: ahci libahci libata tg3 megaraid_sas
crc32c_intel nfit(+) wmi libnvdimm dm_mirror dm_region_hash dm_log
dm_mod
[    2.768590] CR2: 0000000000000370
[    2.771918] ---[ end trace 82a57816a20834cf ]---
[    2.801594] RIP: 0010:bio_add_hw_page+0x4f/0x180
[    2.801606] usb 1-14.4: new high-speed USB device number 4 using xhci_hcd
[    2.806221] Code: 87 f7 00 00 00 0f b7 46 60 45 89 c4 89 cd 49 89
d5 48 89 f3 49 89 fe 66 85 c0 75 5f 66 39 43 62 0f 86 d6 00 00 00 48
8b 53 08 <48> 8b 92 70 03 00 00 48 8b 52 50 8b 92 08 04 00 00 29 ea 39
53 28
[    2.806223] RSP: 0018:ffffbe3300bebba0 EFLAGS: 00010202
[    2.836977] RAX: 0000000000000000 RBX: ffff99d941442b40 RCX: 0000000000000024
[    2.844107] RDX: 0000000000000000 RSI: ffff99d941442b40 RDI: ffff99d9d02b4dd0
[    2.851232] RBP: 0000000000000024 R08: 0000000000000500 R09: 0000000000000200
[    2.858363] R10: 0000000000000024 R11: ffff99d9414442cf R12: 0000000000000500
[    2.865497] R13: ffffe05744131400 R14: ffff99d9d02b4dd0 R15: 0000000000000024
[    2.872629] FS:  0000000000000000(0000) GS:ffff99e87fac0000(0000)
knlGS:0000000000000000
[    2.880715] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.886461] CR2: 0000000000000370 CR3: 0000000132654006 CR4: 00000000007706e0
[    2.888731] usb 1-14.4: New USB device found, idVendor=1604,
idProduct=10c0, bcdDevice= 0.00
[    2.893595] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.893596] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.893597] PKRU: 55555554
[    2.893597] Kernel panic - not syncing: Fatal exception
[    2.902027] usb 1-14.4: New USB device strings: Mfr=0, Product=0,
SerialNumber=0
[    2.916293] Kernel Offset: 0x36800000 from 0xffffffff81000000
(relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[    2.965941] ---[ end Kernel panic - not syncing: Fatal exception ]---

On Sun, Apr 25, 2021 at 12:30 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> bio_max_size() may get called before device_add_disk() and hence needs to
> check whether or not the block device pointer is NULL. Additionally, more
> code needs to be modified than __bio_try_merge_page() to limit the bio size
> to bio_max_size().
>
> This patch prevents that bio_max_size() triggers the following kernel
> crash during a SCSI LUN scan:
>
> BUG: KASAN: null-ptr-deref in bio_add_hw_page+0xa6/0x310
> Read of size 8 at addr 00000000000005a8 by task kworker/u16:0/7
> Workqueue: events_unbound async_run_entry_fn
> Call Trace:
>  show_stack+0x52/0x58
>  dump_stack+0x9d/0xcf
>  kasan_report.cold+0x4b/0x50
>  __asan_load8+0x69/0x90
>  bio_add_hw_page+0xa6/0x310
>  bio_add_pc_page+0xaa/0xe0
>  bio_map_kern+0xdc/0x1a0
>  blk_rq_map_kern+0xcd/0x2d0
>  __scsi_execute+0x9a/0x290 [scsi_mod]
>  scsi_probe_lun.constprop.0+0x17c/0x660 [scsi_mod]
>  scsi_probe_and_add_lun+0x178/0x750 [scsi_mod]
>  __scsi_add_device+0x18c/0x1a0 [scsi_mod]
>  ata_scsi_scan_host+0xe5/0x260 [libata]
>  async_port_probe+0x94/0xb0 [libata]
>  async_run_entry_fn+0x7d/0x2d0
>  process_one_work+0x582/0xac0
>  worker_thread+0x8f/0x5a0
>  kthread+0x222/0x250
>  ret_from_fork+0x1f/0x30
>
> This patch also fixes the following kernel warning:
>
> WARNING: CPU: 1 PID: 15449 at block/bio.c:1034
> __bio_iov_iter_get_pages+0x324/0x350
> Call Trace:
>  bio_iov_iter_get_pages+0x6c/0x360
>  __blkdev_direct_IO_simple+0x291/0x580
>  blkdev_direct_IO+0xb5/0xc0
>  generic_file_direct_write+0x10d/0x290
>  __generic_file_write_iter+0x120/0x290
>  blkdev_write_iter+0x16e/0x280
>  new_sync_write+0x268/0x380
>  vfs_write+0x3e0/0x4f0
>  ksys_write+0xd9/0x180
>  __x64_sys_write+0x43/0x50
>  do_syscall_64+0x32/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Changheun Lee <nanich.lee@samsung.com>
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Fixes: 15050b63567c ("bio: limit bio max size")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/bio.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> Changes compared to v1: included a fix for direct I/O.
>
> diff --git a/block/bio.c b/block/bio.c
> index d718c63b3533..221dc56ba22f 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -257,9 +257,9 @@ EXPORT_SYMBOL(bio_init);
>
>  unsigned int bio_max_size(struct bio *bio)
>  {
> -       struct request_queue *q = bio->bi_bdev->bd_disk->queue;
> +       struct block_device *bdev = bio->bi_bdev;
>
> -       return q->limits.bio_max_bytes;
> +       return bdev ? bdev->bd_disk->queue->limits.bio_max_bytes : UINT_MAX;
>  }
>
>  /**
> @@ -1002,6 +1002,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  {
>         unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
>         unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> +       unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
>         struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
>         struct page **pages = (struct page **)bv;
>         bool same_page = false;
> @@ -1017,7 +1018,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>         BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
>         pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
>
> -       size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> +       size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
> +                                 &offset);
>         if (unlikely(size <= 0))
>                 return size ? size : -EFAULT;
>
>

Re: [PATCH v2] block: Improve limiting the bio size

[Jens Axboe]

On 4/24/21 10:30 PM, Bart Van Assche wrote:
> bio_max_size() may get called before device_add_disk() and hence needs to
> check whether or not the block device pointer is NULL. Additionally, more
> code needs to be modified than __bio_try_merge_page() to limit the bio size
> to bio_max_size().
> 
> This patch prevents that bio_max_size() triggers the following kernel
> crash during a SCSI LUN scan:
> 
> BUG: KASAN: null-ptr-deref in bio_add_hw_page+0xa6/0x310
> Read of size 8 at addr 00000000000005a8 by task kworker/u16:0/7
> Workqueue: events_unbound async_run_entry_fn
> Call Trace:
>  show_stack+0x52/0x58
>  dump_stack+0x9d/0xcf
>  kasan_report.cold+0x4b/0x50
>  __asan_load8+0x69/0x90
>  bio_add_hw_page+0xa6/0x310
>  bio_add_pc_page+0xaa/0xe0
>  bio_map_kern+0xdc/0x1a0
>  blk_rq_map_kern+0xcd/0x2d0
>  __scsi_execute+0x9a/0x290 [scsi_mod]
>  scsi_probe_lun.constprop.0+0x17c/0x660 [scsi_mod]
>  scsi_probe_and_add_lun+0x178/0x750 [scsi_mod]
>  __scsi_add_device+0x18c/0x1a0 [scsi_mod]
>  ata_scsi_scan_host+0xe5/0x260 [libata]
>  async_port_probe+0x94/0xb0 [libata]
>  async_run_entry_fn+0x7d/0x2d0
>  process_one_work+0x582/0xac0
>  worker_thread+0x8f/0x5a0
>  kthread+0x222/0x250
>  ret_from_fork+0x1f/0x30
> 
> This patch also fixes the following kernel warning:
> 
> WARNING: CPU: 1 PID: 15449 at block/bio.c:1034
> __bio_iov_iter_get_pages+0x324/0x350
> Call Trace:
>  bio_iov_iter_get_pages+0x6c/0x360
>  __blkdev_direct_IO_simple+0x291/0x580
>  blkdev_direct_IO+0xb5/0xc0
>  generic_file_direct_write+0x10d/0x290
>  __generic_file_write_iter+0x120/0x290
>  blkdev_write_iter+0x16e/0x280
>  new_sync_write+0x268/0x380
>  vfs_write+0x3e0/0x4f0
>  ksys_write+0xd9/0x180
>  __x64_sys_write+0x43/0x50
>  do_syscall_64+0x32/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae

Since I had to shuffle patches anyway, I folded in this fix. Thanks
Bart.

-- 
Jens Axboe

Re: [PATCH v2] block: Improve limiting the bio size

[Yi Zhang]

Hi Jens/Bart
CKI reproduced the boot panic issue again with the latest
linux-block/for-next[1] today, then I checked the patch 'bio: limit
bio max size'[2] found Bart's fix patch does not fold in that commit,
could you help recheck it, thanks.

[1]
Kernel repo: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
            Commit: ffa77af5731d - Merge branch 'for-5.13/io_uring'
into for-next
[2] bio: limit bio max size
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-next&id=42fb54fbc7072da505c1c59cbe9f8417feb37c27

Thanks
Yi
>
> Since I had to shuffle patches anyway, I folded in this fix. Thanks
> Bart.
>
> --
> Jens Axboe
>

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> Hi Jens/Bart
> CKI reproduced the boot panic issue again with the latest
> linux-block/for-next[1] today, then I checked the patch 'bio: limit
> bio max size'[2] found Bart's fix patch does not fold in that commit,
> could you help recheck it, thanks.
> 
> [1]
> Kernel repo: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
> Commit: ffa77af5731d - Merge branch 'for-5.13/io_uring'
> into for-next
> [2] bio: limit bio max size
> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-next&id=42fb54fbc7072da505c1c59cbe9f8417feb37c27
> 
> Thanks
> Yi
> >
> > Since I had to shuffle patches anyway, I folded in this fix. Thanks
> > Bart.
> >
> > --
> > Jens Axboe
> >

Should we check queue point in bio_max_size()?
__device_add_disk() can be called with "register_queue=false" like as
device_add_disk_no_queue_reg(). How about below?

unsigned int bio_max_size(struct bio *bio)
{
	struct request_queue *q;

	q = (bio->bi_bdev) ? bio->bi_bdev->bd_disk->queue : NULL;
	return q ? q->limits.bio_max_bytes : UINT_MAX;
}

Re: [PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

On 4/26/21 1:34 AM, Changheun Lee wrote:
> Should we check queue point in bio_max_size()?
> __device_add_disk() can be called with "register_queue=false" like as
> device_add_disk_no_queue_reg(). How about below?
> 
> unsigned int bio_max_size(struct bio *bio)
> {
> 	struct request_queue *q;
> 
> 	q = (bio->bi_bdev) ? bio->bi_bdev->bd_disk->queue : NULL;
> 	return q ? q->limits.bio_max_bytes : UINT_MAX;
> }

How could bio_max_size() get called from inside __device_add_disk() if
no request queue is registered? Did I perhaps miss something?

Thanks,

Bart.

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> On 4/26/21 1:34 AM, Changheun Lee wrote:
> > Should we check queue point in bio_max_size()?
> > __device_add_disk() can be called with "register_queue=false" like as
> > device_add_disk_no_queue_reg(). How about below?
> > 
> > unsigned int bio_max_size(struct bio *bio)
> > {
> > 	struct request_queue *q;
> > 
> > 	q = (bio->bi_bdev) ? bio->bi_bdev->bd_disk->queue : NULL;
> > 	return q ? q->limits.bio_max_bytes : UINT_MAX;
> > }
> 
> How could bio_max_size() get called from inside __device_add_disk() if
> no request queue is registered? Did I perhaps miss something?
> 
> Thanks,
> 
> Bart.

__device_add_disk() do not call bio_max_size(). I just imagined bio
operation on disk without request queue. Disk can be added without queue via
device_add_disk_no_queue_reg(). It might be my miss-understood about it.
I didn't check bio operation is possible on disk without request queue yet. 

Thanks,

Changheun Lee.

Re: [PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

On 4/26/21 5:28 PM, Changheun Lee wrote:
> __device_add_disk() do not call bio_max_size(). I just imagined bio
> operation on disk without request queue. Disk can be added without queue via
> device_add_disk_no_queue_reg(). It might be my miss-understood about it.
> I didn't check bio operation is possible on disk without request queue yet. 

Inside __device_add_disk() I found the following:

	WARN_ON_ONCE(!blk_get_queue(disk->queue));

I'm not sure how that could work without initializing disk->queue first?

Thanks,

Bart.

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> On 4/26/21 5:28 PM, Changheun Lee wrote:
> > __device_add_disk() do not call bio_max_size(). I just imagined bio
> > operation on disk without request queue. Disk can be added without queue via
> > device_add_disk_no_queue_reg(). It might be my miss-understood about it.
> > I didn't check bio operation is possible on disk without request queue yet. 
> 
> Inside __device_add_disk() I found the following:
> 
> WARN_ON_ONCE(!blk_get_queue(disk->queue));
> 
> I'm not sure how that could work without initializing disk->queue first?
> 
> Thanks,
> 
> Bart.

I think so. But I just asked whether queue should be checked, or not.

Anyway we should determine which pointer variables must be checked in
bio_max_size(). Candidates are bio->bi_bdev, bi_bdev->bd_disk, and
bd_disk->queue I think.

Actually I checked "bio->bi_disk" at first as below. It works well.

unsigned int bio_max_size(struct bio *bio)                                                                                                                                                             
{
	struct request_queue *q;

	if (!bio->bi_disk)
		return UINT_MAX;

	q = bio->bi_disk->queue;
	return q->limits.bio_max_bytes;
}

But I removed bi_disk check condition after applying of Christoph's patch.
Because I got a feedback that is not needed more.

Without bi_disk checking, booting was failed like now. call stack was below.

[   13.341949] pc : __bio_add_pc_page+0x120/0x190
[   13.341951] lr : bio_copy_kern+0xf8/0x20c
[   13.341953] sp : ffffffc012e23950
[   13.341954] x29: ffffffc012e23950 x28: 0000000000001000 
[   13.341955] x27: ffffffff209fa140 x26: 0000000000000200 
[   13.341957] x25: ffffff888d151400 x24: ffffff8919479830 
[   13.341959] x23: ffffff8919479830 x22: ffffffff209fa140 
[   13.341960] x21: 0000000000000000 x20: ffffff882db2f300 
[   13.341962] x19: 0000000000000200 x18: ffffffc012e65048 
[   13.341963] x17: 0000000000000000 x16: 00000000000002fe 
[   13.341965] x15: 0000000000000000 x14: 0000000005bae400 
[   13.341966] x13: 00000000000019f2 x12: ffffffc912fb2000 
[   13.341968] x11: 0000000000000000 x10: 000000000002784d 
[   13.341970] x9 : 0000000000000000 x8 : 0000000000000000 
[   13.341971] x7 : 0000000000000000 x6 : 000000000000003f 
[   13.341973] x5 : ffffffc012e23994 x4 : 0000000000000000 
[   13.341974] x3 : 0000000000000200 x2 : ffffffff209fa140 
[   13.341976] x1 : ffffff882db2f300 x0 : ffffff8919479830 
[   13.341977] Call trace:
[   13.341979] __bio_add_pc_page+0x120/0x190
[   13.341981] bio_copy_kern+0xf8/0x20c
[   13.341984] blk_rq_map_kern+0xa4/0x154
[   13.341985] __scsi_execute+0x88/0x1c0
[   13.341988] srpmb_scsi_ioctl+0x264/0x444 [scsi_srpmb]
[   13.341990] srpmb_worker+0x134/0x5f8 [scsi_srpmb]
[   13.341992] process_one_work+0x2f8/0x5b8
[   13.341994] worker_thread+0x28c/0x518
[   13.341996] kthread+0x16c/0x17c
[   13.341998] ret_from_fork+0x10/0x18

I think "bi_bdev->bd_disk" checking might be needed too.

Thanks,

Changheun Lee

Re: [PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

On 4/26/21 9:12 PM, Changheun Lee wrote:
> Actually I checked "bio->bi_disk" at first as below. It works well.

Agreed that the bi_disk pointer needs to be checked.

> I think "bi_bdev->bd_disk" checking might be needed too.

bio_max_size() occurs in the hot path. Since if-tests have a runtime
cost I'd like to keep the number of if-tests inside bio_max_size() to a
minimum.

I only found one bd_disk assignment in the kernel tree, namely inside
bdev_alloc(). The two bdev_alloc() calls I found inside the kernel tree
pass a valid disk pointer to bdev_alloc(). So I think it is not
necessary to check the disk pointer inside bio_max_size().

Thanks,

Bart.

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> On 4/26/21 9:12 PM, Changheun Lee wrote:
> > Actually I checked "bio->bi_disk" at first as below. It works well.
> 
> Agreed that the bi_disk pointer needs to be checked.
> 
> > I think "bi_bdev->bd_disk" checking might be needed too.
> 
> bio_max_size() occurs in the hot path. Since if-tests have a runtime
> cost I'd like to keep the number of if-tests inside bio_max_size() to a
> minimum.
> 
> I only found one bd_disk assignment in the kernel tree, namely inside
> bdev_alloc(). The two bdev_alloc() calls I found inside the kernel tree
> pass a valid disk pointer to bdev_alloc(). So I think it is not
> necessary to check the disk pointer inside bio_max_size().
> 
> Thanks,
> 
> Bart.

I think bio->bi_bdev should be null in bio_copy_kern(), or bio_map_kern()
logically. Because it is simple buffer mapping in driver layer. Actually
bio->bi_bdev is null in my test environment. So bio->bi_bdev check only is
enough as Bart's patch.

Actually I don't know why NULL pointer dereference is occurred with Bart's
patch in blk_rq_map_kern(). And same problem have not occured yet in my
test environment with Bart's patch.
Maybe I missed something, or missunderstood?

Thanks,

Changheun Lee.

Re: [PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

On 4/28/21 12:21 AM, Changheun Lee wrote:
> Actually I don't know why NULL pointer dereference is occurred with Bart's
> patch in blk_rq_map_kern(). And same problem have not occured yet in my
> test environment with Bart's patch.
> Maybe I missed something, or missunderstood?

The call trace that I shared on the linux-block mailing list was
encountered inside a VM that has a SATA disk attached to it and also a
SATA CD-ROM. Does replicating that setup allow to reproduce the NULL
pointer dereference that I reported?

Bart.

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> On 4/28/21 12:21 AM, Changheun Lee wrote:
> > Actually I don't know why NULL pointer dereference is occurred with Bart's
> > patch in blk_rq_map_kern(). And same problem have not occured yet in my
> > test environment with Bart's patch.
> > Maybe I missed something, or missunderstood?
> 
> The call trace that I shared on the linux-block mailing list was
> encountered inside a VM that has a SATA disk attached to it and also a
> SATA CD-ROM. Does replicating that setup allow to reproduce the NULL
> pointer dereference that I reported?
> 
> Bart.

I can reproduce and see NULL pointer dereference in same call path without
your patch. But I can't reproduced it with your patch.
I tested in ubuntu via grub. SATA disk is attached only in my environment.
Actually I didn't test in VM yet. I'll try.

Thanks,

Changheun.

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> > On 4/28/21 12:21 AM, Changheun Lee wrote:
> > > Actually I don't know why NULL pointer dereference is occurred with Bart's
> > > patch in blk_rq_map_kern(). And same problem have not occured yet in my
> > > test environment with Bart's patch.
> > > Maybe I missed something, or missunderstood?
> > 
> > The call trace that I shared on the linux-block mailing list was
> > encountered inside a VM that has a SATA disk attached to it and also a
> > SATA CD-ROM. Does replicating that setup allow to reproduce the NULL
> > pointer dereference that I reported?
> > 
> > Bart.
> 
> I can reproduce and see NULL pointer dereference in same call path without
> your patch. But I can't reproduced it with your patch.
> I tested in ubuntu via grub. SATA disk is attached only in my environment.
> Actually I didn't test in VM yet. I'll try.
> 
> Thanks,
> 
> Changheun.

NULL pointer dereference is not produced in my VM environment too.

Dear Bart, Yi
I'll prepare v9 patch including Bart's modification.
Could you please test it on your environment?

Re: [PATCH v2] block: Improve limiting the bio size

[Bart Van Assche]

On 5/3/21 2:28 AM, Changheun Lee wrote:
> NULL pointer dereference is not produced in my VM environment too.
> 
> Dear Bart, Yi
> I'll prepare v9 patch including Bart's modification.
> Could you please test it on your environment?

v9 does not cause a change in behavior for blktests on my test setup. It
would be great if someone could rerun xfstests on a setup where this patch
has been applied.

Thanks,

Bart.

Re: [PATCH v2] block: Improve limiting the bio size

[Jens Axboe]

On 4/26/21 12:32 AM, Yi Zhang wrote:
> Hi Jens/Bart
> CKI reproduced the boot panic issue again with the latest
> linux-block/for-next[1] today, then I checked the patch 'bio: limit
> bio max size'[2] found Bart's fix patch does not fold in that commit,
> could you help recheck it, thanks.

Hmm, maybe I botched it. But a bit too much churn around this patch,
Changheun maybe you can resend a fully tested version?

-- 
Jens Axboe

Re: [PATCH v2] block: Improve limiting the bio size

[Changheun Lee]

> On 4/26/21 12:32 AM, Yi Zhang wrote:
> > Hi Jens/Bart
> > CKI reproduced the boot panic issue again with the latest
> > linux-block/for-next[1] today, then I checked the patch 'bio: limit
> > bio max size'[2] found Bart's fix patch does not fold in that commit,
> > could you help recheck it, thanks.
> 
> Hmm, maybe I botched it. But a bit too much churn around this patch,
> Changheun maybe you can resend a fully tested version?
> 
> -- 
> Jens Axboe

I'm really sorry Jens, and to all. I'll check and resend. Sorry again.

Changheun Lee.