On Mon, May 10, 2021 at 05:05:35PM -0700, Omar Sandoval wrote: > From: Omar Sandoval <osandov@xxxxxx> > > __blk_mq_sched_bio_merge() gets the ctx and hctx for the current CPU and > passes the hctx to ->bio_merge(). kyber_bio_merge() then gets the ctx > for the current CPU again and uses that to get the corresponding Kyber > context in the passed hctx. However, the thread may be preempted between > the two calls to blk_mq_get_ctx(), and the ctx returned the second time > may no longer correspond to the passed hctx. This "works" accidentally > most of the time, but it can cause us to read garbage if the second ctx > came from an hctx with more ctx's than the first one (i.e., if > ctx->index_hw[hctx->type] > hctx->nr_ctx). > > This manifested as this UBSAN array index out of bounds error reported > by Jakub: > > UBSAN: array-index-out-of-bounds in ../kernel/locking/qspinlock.c:130:9 > index 13106 is out of range for type 'long unsigned int [128]' > Call Trace: > dump_stack+0xa4/0xe5 > ubsan_epilogue+0x5/0x40 > __ubsan_handle_out_of_bounds.cold.13+0x2a/0x34 > queued_spin_lock_slowpath+0x476/0x480 > do_raw_spin_lock+0x1c2/0x1d0 > kyber_bio_merge+0x112/0x180 > blk_mq_submit_bio+0x1f5/0x1100 > submit_bio_noacct+0x7b0/0x870 > submit_bio+0xc2/0x3a0 > btrfs_map_bio+0x4f0/0x9d0 > btrfs_submit_data_bio+0x24e/0x310 > submit_one_bio+0x7f/0xb0 > submit_extent_page+0xc4/0x440 > __extent_writepage_io+0x2b8/0x5e0 > __extent_writepage+0x28d/0x6e0 > extent_write_cache_pages+0x4d7/0x7a0 > extent_writepages+0xa2/0x110 > do_writepages+0x8f/0x180 > __writeback_single_inode+0x99/0x7f0 > writeback_sb_inodes+0x34e/0x790 > __writeback_inodes_wb+0x9e/0x120 > wb_writeback+0x4d2/0x660 > wb_workfn+0x64d/0xa10 > process_one_work+0x53a/0xa80 > worker_thread+0x69/0x5b0 > kthread+0x20b/0x240 > ret_from_fork+0x1f/0x30 > > Only Kyber uses the hctx, so fix it by passing the request_queue to > ->bio_merge() instead. BFQ and mq-deadline just use that, and Kyber can > map the queues itself to avoid the mismatch. I should elaborate that once we get the Kyber context, it doesn't matter if we get preempted after that, since we lock it for merging.
