Re: [PATCH] block: fix bio-allocation from per-cpu cache

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Thu, Oct 27, 2022 at 11:38:50AM +0100, Pavel Begunkov wrote:
>On 10/27/22 11:04, Kanchan Joshi wrote:
>>If cache does not have any entry, make sure to detect that and return
>>failure. Otherwise this leads to null pointer dereference.
>
>Damn, it was done right in v2
>
>https://lore.kernel.org/all/9fd04486d972c1f3ef273fa26b4b6bf51a5e4270.1666122465.git.asml.silence@gmail.com/
>
>Perhaps I based v3 on a wrong version. Thanks
>
>
>>Fixes: 13a184e26965 ("block/bio: add pcpu caching for non-polling bio_put")
>>Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>---
>>Can be reproduced by:
>>fio -direct=1 -iodepth=1 -rw=randread -ioengine=io_uring -bs=4k -numjobs=1 -size=4k -filename=/dev/nvme0n1 -hipri=1 -name=block
>>
>>BUG: KASAN: null-ptr-deref in bio_alloc_bioset.cold+0x2a/0x16a
>>Read of size 8 at addr 0000000000000000 by task fio/1835
>>
>>CPU: 5 PID: 1835 Comm: fio Not tainted 6.1.0-rc2+ #226
>>Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-0-g
>>Call Trace:
>>  <TASK>
>>  dump_stack_lvl+0x34/0x48
>>  print_report+0x490/0x4a1
>>  ? __virt_addr_valid+0x28/0x140
>>  ? bio_alloc_bioset.cold+0x2a/0x16a
>>  kasan_report+0xb3/0x130
>>  ? bio_alloc_bioset.cold+0x2a/0x16a
>>  bio_alloc_bioset.cold+0x2a/0x16a
>>  ? bvec_alloc+0xf0/0xf0
>>  ? iov_iter_is_aligned+0x130/0x2c0
>>  blkdev_direct_IO.part.0+0x16a/0x8d0
>>
>>  block/bio.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>>diff --git a/block/bio.c b/block/bio.c
>>index 8f624ffaf3d0..66f088bb3736 100644
>>--- a/block/bio.c
>>+++ b/block/bio.c
>>@@ -439,13 +439,14 @@ static struct bio *bio_alloc_percpu_cache(struct block_device *bdev,
>>  	cache = per_cpu_ptr(bs->cache, get_cpu());
>>  	if (!cache->free_list &&
>>-	    READ_ONCE(cache->nr_irq) >= ALLOC_CACHE_THRESHOLD) {
>>+	    READ_ONCE(cache->nr_irq) >= ALLOC_CACHE_THRESHOLD)
>>  		bio_alloc_irq_cache_splice(cache);
>>-		if (!cache->free_list) {
>>-			put_cpu();
>>-			return NULL;
>>-		}
>>+
>>+	if (!cache->free_list) {
>
>Let's nest it under the other "if (!cache->free_list)"

Not sure if I got you. It was under that if condition earlier, and that
part causes trouble.
What you wrote in v2 is another way, but there also we have two checks
on cache->free_list.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]