* [PATCH v2] block: Improve limiting the bio size @ 2021-04-25 4:30 Bart Van Assche 2021-04-25 7:36 ` Yi Zhang 2021-04-25 16:19 ` Jens Axboe 0 siblings, 2 replies; 17+ messages in thread From: Bart Van Assche @ 2021-04-25 4:30 UTC (permalink / raw) To: Jens Axboe Cc: linux-block, Christoph Hellwig, Bart Van Assche, Ming Lei, Changheun Lee, Jaegeuk Kim 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; ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-25 4:30 [PATCH v2] block: Improve limiting the bio size Bart Van Assche @ 2021-04-25 7:36 ` Yi Zhang 2021-04-25 16:19 ` Jens Axboe 1 sibling, 0 replies; 17+ messages in thread From: Yi Zhang @ 2021-04-25 7:36 UTC (permalink / raw) To: Bart Van Assche Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei, Changheun Lee, Jaegeuk Kim, Bruno Goncalves 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; > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-25 4:30 [PATCH v2] block: Improve limiting the bio size Bart Van Assche 2021-04-25 7:36 ` Yi Zhang @ 2021-04-25 16:19 ` Jens Axboe 2021-04-26 6:32 ` Yi Zhang 1 sibling, 1 reply; 17+ messages in thread From: Jens Axboe @ 2021-04-25 16:19 UTC (permalink / raw) To: Bart Van Assche Cc: linux-block, Christoph Hellwig, Ming Lei, Changheun Lee, Jaegeuk Kim 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-25 16:19 ` Jens Axboe @ 2021-04-26 6:32 ` Yi Zhang [not found] ` <CGME20210426085241epcas1p46ed8de18a98c40218dacd58fc4b25ff9@epcas1p4.samsung.com> 2021-04-26 12:54 ` Jens Axboe 0 siblings, 2 replies; 17+ messages in thread From: Yi Zhang @ 2021-04-26 6:32 UTC (permalink / raw) To: Jens Axboe Cc: Bart Van Assche, linux-block, Christoph Hellwig, Ming Lei, Changheun Lee, Jaegeuk Kim, Bruno Goncalves 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210426085241epcas1p46ed8de18a98c40218dacd58fc4b25ff9@epcas1p4.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210426085241epcas1p46ed8de18a98c40218dacd58fc4b25ff9@epcas1p4.samsung.com> @ 2021-04-26 8:34 ` Changheun Lee 2021-04-26 16:17 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-04-26 8:34 UTC (permalink / raw) To: bvanassche; +Cc: yi.zhang, axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei > 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; } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-26 8:34 ` Changheun Lee @ 2021-04-26 16:17 ` Bart Van Assche [not found] ` <CGME20210427004655epcas1p15cc4b2be6312c4762272474908607722@epcas1p1.samsung.com> 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2021-04-26 16:17 UTC (permalink / raw) To: Changheun Lee Cc: yi.zhang, axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210427004655epcas1p15cc4b2be6312c4762272474908607722@epcas1p1.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210427004655epcas1p15cc4b2be6312c4762272474908607722@epcas1p1.samsung.com> @ 2021-04-27 0:28 ` Changheun Lee 2021-04-27 2:46 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-04-27 0:28 UTC (permalink / raw) To: bvanassche Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, nanich.lee, yi.zhang > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-27 0:28 ` Changheun Lee @ 2021-04-27 2:46 ` Bart Van Assche [not found] ` <CGME20210427043022epcas1p47f11139bc1e08925bcbbdca79e5c8e36@epcas1p4.samsung.com> 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2021-04-27 2:46 UTC (permalink / raw) To: Changheun Lee Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, yi.zhang 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210427043022epcas1p47f11139bc1e08925bcbbdca79e5c8e36@epcas1p4.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210427043022epcas1p47f11139bc1e08925bcbbdca79e5c8e36@epcas1p4.samsung.com> @ 2021-04-27 4:12 ` Changheun Lee 2021-04-27 14:59 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-04-27 4:12 UTC (permalink / raw) To: bvanassche Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, nanich.lee, yi.zhang > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-27 4:12 ` Changheun Lee @ 2021-04-27 14:59 ` Bart Van Assche [not found] ` <CGME20210428073921epcas1p2b161b5ccc9d7ec61c1200155da95c5b9@epcas1p2.samsung.com> 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2021-04-27 14:59 UTC (permalink / raw) To: Changheun Lee Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, yi.zhang 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210428073921epcas1p2b161b5ccc9d7ec61c1200155da95c5b9@epcas1p2.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210428073921epcas1p2b161b5ccc9d7ec61c1200155da95c5b9@epcas1p2.samsung.com> @ 2021-04-28 7:21 ` Changheun Lee 2021-04-28 15:52 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-04-28 7:21 UTC (permalink / raw) To: bvanassche Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, nanich.lee, yi.zhang > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-28 7:21 ` Changheun Lee @ 2021-04-28 15:52 ` Bart Van Assche [not found] ` <CGME20210429065701epcas1p363667a6f0c598f27bb5afde32473ea39@epcas1p3.samsung.com> 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2021-04-28 15:52 UTC (permalink / raw) To: Changheun Lee Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, yi.zhang 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210429065701epcas1p363667a6f0c598f27bb5afde32473ea39@epcas1p3.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210429065701epcas1p363667a6f0c598f27bb5afde32473ea39@epcas1p3.samsung.com> @ 2021-04-29 6:39 ` Changheun Lee [not found] ` <CGME20210503094654epcas1p3ce7761e2f0fc304d1c08a9b0bf0485ff@epcas1p3.samsung.com> 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-04-29 6:39 UTC (permalink / raw) To: bvanassche Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei, nanich.lee, yi.zhang > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210503094654epcas1p3ce7761e2f0fc304d1c08a9b0bf0485ff@epcas1p3.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210503094654epcas1p3ce7761e2f0fc304d1c08a9b0bf0485ff@epcas1p3.samsung.com> @ 2021-05-03 9:28 ` Changheun Lee 2021-05-03 16:35 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Changheun Lee @ 2021-05-03 9:28 UTC (permalink / raw) To: bvanassche, yi.zhang; +Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei > > 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-05-03 9:28 ` Changheun Lee @ 2021-05-03 16:35 ` Bart Van Assche 0 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2021-05-03 16:35 UTC (permalink / raw) To: Changheun Lee, yi.zhang Cc: axboe, bgoncalv, hch, jaegeuk, linux-block, ming.lei 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] block: Improve limiting the bio size 2021-04-26 6:32 ` Yi Zhang [not found] ` <CGME20210426085241epcas1p46ed8de18a98c40218dacd58fc4b25ff9@epcas1p4.samsung.com> @ 2021-04-26 12:54 ` Jens Axboe [not found] ` <CGME20210427000106epcas1p3f39e318e9211bf3378b3e4afad2de56e@epcas1p3.samsung.com> 1 sibling, 1 reply; 17+ messages in thread From: Jens Axboe @ 2021-04-26 12:54 UTC (permalink / raw) To: Yi Zhang Cc: Bart Van Assche, linux-block, Christoph Hellwig, Ming Lei, Changheun Lee, Jaegeuk Kim, Bruno Goncalves 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CGME20210427000106epcas1p3f39e318e9211bf3378b3e4afad2de56e@epcas1p3.samsung.com>]
* Re: [PATCH v2] block: Improve limiting the bio size [not found] ` <CGME20210427000106epcas1p3f39e318e9211bf3378b3e4afad2de56e@epcas1p3.samsung.com> @ 2021-04-26 23:43 ` Changheun Lee 0 siblings, 0 replies; 17+ messages in thread From: Changheun Lee @ 2021-04-26 23:43 UTC (permalink / raw) To: axboe Cc: bgoncalv, bvanassche, hch, jaegeuk, linux-block, ming.lei, nanich.lee, yi.zhang > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-05-03 16:36 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-25 4:30 [PATCH v2] block: Improve limiting the bio size Bart Van Assche 2021-04-25 7:36 ` Yi Zhang 2021-04-25 16:19 ` Jens Axboe 2021-04-26 6:32 ` Yi Zhang [not found] ` <CGME20210426085241epcas1p46ed8de18a98c40218dacd58fc4b25ff9@epcas1p4.samsung.com> 2021-04-26 8:34 ` Changheun Lee 2021-04-26 16:17 ` Bart Van Assche [not found] ` <CGME20210427004655epcas1p15cc4b2be6312c4762272474908607722@epcas1p1.samsung.com> 2021-04-27 0:28 ` Changheun Lee 2021-04-27 2:46 ` Bart Van Assche [not found] ` <CGME20210427043022epcas1p47f11139bc1e08925bcbbdca79e5c8e36@epcas1p4.samsung.com> 2021-04-27 4:12 ` Changheun Lee 2021-04-27 14:59 ` Bart Van Assche [not found] ` <CGME20210428073921epcas1p2b161b5ccc9d7ec61c1200155da95c5b9@epcas1p2.samsung.com> 2021-04-28 7:21 ` Changheun Lee 2021-04-28 15:52 ` Bart Van Assche [not found] ` <CGME20210429065701epcas1p363667a6f0c598f27bb5afde32473ea39@epcas1p3.samsung.com> 2021-04-29 6:39 ` Changheun Lee [not found] ` <CGME20210503094654epcas1p3ce7761e2f0fc304d1c08a9b0bf0485ff@epcas1p3.samsung.com> 2021-05-03 9:28 ` Changheun Lee 2021-05-03 16:35 ` Bart Van Assche 2021-04-26 12:54 ` Jens Axboe [not found] ` <CGME20210427000106epcas1p3f39e318e9211bf3378b3e4afad2de56e@epcas1p3.samsung.com> 2021-04-26 23:43 ` Changheun Lee
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.