Re: [PATCH] kyber: fix out of bounds access when preempted

From: Jens Axboe <axboe@kernel.dk>
To: Omar Sandoval <osandov@osandov.com>, linux-block@vger.kernel.org
Cc: kernel-team@fb.com, Jakub Kicinski <kuba@kernel.org>,
	Jianchao Wang <jianchao.w.wang@oracle.com>
Subject: Re: [PATCH] kyber: fix out of bounds access when preempted
Date: Tue, 11 May 2021 08:12:31 -0600	[thread overview]
Message-ID: <f22b2f93-c6da-fa1e-dc34-ea797caac2f6@kernel.dk> (raw)
In-Reply-To: <c7598605401a48d5cfeadebb678abd10af22b83f.1620691329.git.osandov@fb.com>

On 5/10/21 6:05 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> __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.

Applied, thanks Omar.

-- 
Jens Axboe