Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] bfq: Check if bfqq is NULL in bfq_insert_request
To:     Paolo Valente <paolo.valente@linaro.org>
Cc:     Jens Axboe <axboe@kernel.dk>,
        linux-block <linux-block@vger.kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Hsin-Yi Wang <hsinyi@google.com>,
        Nicolas Boichat <drinkcat@chromium.org>,
        Doug Anderson <dianders@chromium.org>
References: <1563816648-12057-1-git-send-email-linux@roeck-us.net>
 <20190728151931.GA29181@roeck-us.net>
 <0BCD5EDA-6D08-4023-9EEA-087F0AB99D47@linaro.org>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <23890163-facd-3838-ddee-770b7c2f32ea@roeck-us.net>
Date:   Tue, 30 Jul 2019 06:35:28 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <0BCD5EDA-6D08-4023-9EEA-087F0AB99D47@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 7/30/19 1:55 AM, Paolo Valente wrote:
> Hi Guenter,
> sorry for the delay (Dolomiti's fault).
> 
> I didn't consider that rq->elv-icq might have been NULL also
> because of OOM.  Thanks for spotting this issue.
> 
> As for the other places where the return value of bfq_init_rq is used,
> unfortunately I think they matter too.  Those other places are related
> to request merging, which is the alternative destiny of requests
> (instead of being just inserted).  But, regardless of whether a
> request is to be merged or inserted, that request may be destined to a
> bfq_queue (possibly merged with a request already in a bfq_queue), and
> a NULL return value by bfq_init_rq leads to a crash.  I guess you can
> reproduce your failure also for the merge case, by generating
> sequential, direct I/O with queue depth > 1, and of course by enabling
> failslab.
> 
My assumption was that requests would only be merged if they are associated
with the same io context. In that case, that IO context isn't reallocated
with ioc_create_icq() but reused, and icq would thus never be NULL.
I guess that assumption was wrong.

> If my considerations above are correct, do you want to propose a
> complete fix yourself?
> 

Sure, I'll send an updated patch.

Thanks,
Guenter

> Thanks,
> Paolo
> 
>> Il giorno 28 lug 2019, alle ore 17:19, Guenter Roeck <linux@roeck-us.net> ha scritto:
>>
>> ping ... just in case this patch got lost in Paolo's queue.
>>
>> Guenter
>>
>> On Mon, Jul 22, 2019 at 10:30:48AM -0700, Guenter Roeck wrote:
>>> In bfq_insert_request(), bfqq is initialized with:
>>> 	bfqq = bfq_init_rq(rq);
>>> In bfq_init_rq(), we find:
>>> 	if (unlikely(!rq->elv.icq))
>>> 		return NULL;
>>> Indeed, rq->elv.icq can be NULL if the memory allocation in
>>> create_task_io_context() failed.
>>>
>>> A comment in bfq_insert_request() suggests that bfqq is supposed to be
>>> non-NULL if 'at_head || blk_rq_is_passthrough(rq)' is false. Yet, as
>>> debugging and practical experience shows, this is not the case in the
>>> above situation.
>>>
>>> This results in the following crash.
>>>
>>> Unable to handle kernel NULL pointer dereference
>>> 	at virtual address 00000000000001b0
>>> ...
>>> Call trace:
>>> bfq_setup_cooperator+0x44/0x134
>>> bfq_insert_requests+0x10c/0x630
>>> blk_mq_sched_insert_requests+0x60/0xb4
>>> blk_mq_flush_plug_list+0x290/0x2d4
>>> blk_flush_plug_list+0xe0/0x230
>>> blk_finish_plug+0x30/0x40
>>> generic_writepages+0x60/0x94
>>> blkdev_writepages+0x24/0x30
>>> do_writepages+0x74/0xac
>>> __filemap_fdatawrite_range+0x94/0xc8
>>> file_write_and_wait_range+0x44/0xa0
>>> blkdev_fsync+0x38/0x68
>>> vfs_fsync_range+0x68/0x80
>>> do_fsync+0x44/0x80
>>>
>>> The problem is relatively easy to reproduce by running an image with
>>> failslab enabled, such as with:
>>>
>>> cd /sys/kernel/debug/failslab
>>> echo 10 > probability
>>> echo 300 > times
>>>
>>> Avoid the problem by checking if bfqq is NULL before using it. With the
>>> NULL check in place, requests with missing io context are queued
>>> immediately, and the crash is no longer seen.
>>>
>>> Fixes: 18e5a57d79878 ("block, bfq: postpone rq preparation to insert or merge")
>>> Reported-by: Hsin-Yi Wang  <hsinyi@google.com>
>>> Cc: Hsin-Yi Wang <hsinyi@google.com>
>>> Cc: Nicolas Boichat <drinkcat@chromium.org>
>>> Cc: Doug Anderson <dianders@chromium.org>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> block/bfq-iosched.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index 72860325245a..56f3f4227010 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -5417,7 +5417,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>>
>>> 	spin_lock_irq(&bfqd->lock);
>>> 	bfqq = bfq_init_rq(rq);
>>> -	if (at_head || blk_rq_is_passthrough(rq)) {
>>> +	if (!bfqq || at_head || blk_rq_is_passthrough(rq)) {
>>> 		if (at_head)
>>> 			list_add(&rq->queuelist, &bfqd->dispatch);
>>> 		else
>>> -- 
>>> 2.7.4
>>>
> 
>