Re: 5.14.0-rc1 KASAN use after free

[Jens Axboe]

On 7/18/21 3:08 PM, Oleksandr Natalenko wrote:
> + Paolo, Jens et al.
> 
> On čtvrtek 15. července 2021 16:32:29 CEST jim.cromie@xxxxxxxxx wrote:
>> hi all,
>>
>> I noticed this report this morning, from 3 days ago,
>> about 10 minutes after boot.
>> Its easiest to ignore it, and I dont want to make a fuss,
>> but it looks useful to someone
>>
>>
>> [   33.663464] Bluetooth: RFCOMM ver 1.11
>> [  646.343628]
>> ================================================================== [ 
>> 646.343649] BUG: KASAN: use-after-free in bfq_get_queue+0x47d/0x900 [ 
>> 646.343680] Read of size 8 at addr ffff88810d864a00 by task
>> journal-offline/1639

There are only a few commits between 5.13 and master in this area, see
attached. I'd just start reverting from the top, one by one, and see
which one is causing the issue. Jim, would that be feasible?

-- 
Jens Axboe

commit fd2ef39cc9a6b9c4c41864ac506906c52f94b06a
Author: Jan Kara <jack@xxxxxxx>
Date:   Wed Jun 23 11:36:34 2021 +0200

    blk: Fix lock inversion between ioc lock and bfqd lock
    
    Lockdep complains about lock inversion between ioc->lock and bfqd->lock:
    
    bfqd -> ioc:
     put_io_context+0x33/0x90 -> ioc->lock grabbed
     blk_mq_free_request+0x51/0x140
     blk_put_request+0xe/0x10
     blk_attempt_req_merge+0x1d/0x30
     elv_attempt_insert_merge+0x56/0xa0
     blk_mq_sched_try_insert_merge+0x4b/0x60
     bfq_insert_requests+0x9e/0x18c0 -> bfqd->lock grabbed
     blk_mq_sched_insert_requests+0xd6/0x2b0
     blk_mq_flush_plug_list+0x154/0x280
     blk_finish_plug+0x40/0x60
     ext4_writepages+0x696/0x1320
     do_writepages+0x1c/0x80
     __filemap_fdatawrite_range+0xd7/0x120
     sync_file_range+0xac/0xf0
    
    ioc->bfqd:
     bfq_exit_icq+0xa3/0xe0 -> bfqd->lock grabbed
     put_io_context_active+0x78/0xb0 -> ioc->lock grabbed
     exit_io_context+0x48/0x50
     do_exit+0x7e9/0xdd0
     do_group_exit+0x54/0xc0
    
    To avoid this inversion we change blk_mq_sched_try_insert_merge() to not
    free the merged request but rather leave that upto the caller similarly
    to blk_mq_sched_try_merge(). And in bfq_insert_requests() we make sure
    to free all the merged requests after dropping bfqd->lock.
    
    Fixes: aee69d78dec0 ("block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler")
    Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
    Acked-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Signed-off-by: Jan Kara <jack@xxxxxxx>
    Link: https://lore.kernel.org/r/20210623093634.27879-3-jack@xxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit a921c655f2033dd1ce1379128efe881dda23ea37
Author: Jan Kara <jack@xxxxxxx>
Date:   Wed Jun 23 11:36:33 2021 +0200

    bfq: Remove merged request already in bfq_requests_merged()
    
    Currently, bfq does very little in bfq_requests_merged() and handles all
    the request cleanup in bfq_finish_requeue_request() called from
    blk_mq_free_request(). That is currently safe only because
    blk_mq_free_request() is called shortly after bfq_requests_merged()
    while bfqd->lock is still held. However to fix a lock inversion between
    bfqd->lock and ioc->lock, we need to call blk_mq_free_request() after
    dropping bfqd->lock. That would mean that already merged request could
    be seen by other processes inside bfq queues and possibly dispatched to
    the device which is wrong. So move cleanup of the request from
    bfq_finish_requeue_request() to bfq_requests_merged().
    
    Acked-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Signed-off-by: Jan Kara <jack@xxxxxxx>
    Link: https://lore.kernel.org/r/20210623093634.27879-2-jack@xxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit 9a2ac41b13c573703d6689f51f3e27dd658324be
Author: Paolo Valente <paolo.valente@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:48 2021 +0200

    block, bfq: reset waker pointer with shared queues
    
    Commit 85686d0dc194 ("block, bfq: keep shared queues out of the waker
    mechanism") leaves shared bfq_queues out of the waker-detection
    mechanism. It attains this goal by not updating the pointer
    last_completed_rq_bfqq, if the last request completed belongs to a
    shared bfq_queue (so that the pointer will not point to the shared
    bfq_queue).
    
    Yet this has a side effect: the pointer last_completed_rq_bfqq keeps
    pointing, deceptively, to a bfq_queue that actually is not the last
    one to have had a request completed. As a consequence, such a
    bfq_queue may deceptively be considered as a waker of some bfq_queue,
    even of some shared bfq_queue.
    
    To address this issue, reset last_completed_rq_bfqq if the last
    request completed belongs to a shared queue.
    
    Fixes: 85686d0dc194 ("block, bfq: keep shared queues out of the waker mechanism")
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-8-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit efc72524b3a9e4e7bc7c07f756528736409ec1b7
Author: Paolo Valente <paolo.valente@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:47 2021 +0200

    block, bfq: check waker only for queues with no in-flight I/O
    
    Consider two bfq_queues, say Q1 and Q2, with Q2 empty. If a request of
    Q1 gets completed shortly before a new request arrives for Q2, then
    BFQ flags Q1 as a candidate waker for Q2. Yet, the arrival of this new
    request may have a different cause, in the following case. If also Q2
    has requests in flight while waiting for the arrival of a new request,
    then the completion of its own requests may be the actual cause of the
    awakening of the process that sends I/O to Q2. So Q1 may be flagged
    wrongly as a candidate waker.
    
    This commit avoids this deceptive flagging, by disabling
    candidate-waker flagging for Q2, if Q2 has in-flight I/O.
    
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-7-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit bd3664b362381c4c1473753ebedf0ab242a60d1d
Author: Paolo Valente <paolo.valente@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:46 2021 +0200

    block, bfq: avoid delayed merge of async queues
    
    Since commit 430a67f9d616 ("block, bfq: merge bursts of newly-created
    queues"), BFQ may schedule a merge between a newly created sync
    bfq_queue, say Q2, and the last sync bfq_queue created, say Q1. To this
    goal, BFQ stores the address of Q1 in the field bic->stable_merge_bfqq
    of the bic associated with Q2. So, when the time for the possible merge
    arrives, BFQ knows which bfq_queue to merge Q2 with. In particular,
    BFQ checks for possible merges on request arrivals.
    
    Yet the same bic may also be associated with an async bfq_queue, say
    Q3. So, if a request for Q3 arrives, then the above check may happen
    to be executed while the bfq_queue at hand is Q3, instead of Q2. In
    this case, Q1 happens to be merged with an async bfq_queue. This is
    not only a conceptual mistake, because async queues are to be kept out
    of queue merging, but also a bug that leads to inconsistent states.
    
    This commits simply filters async queues out of delayed merges.
    
    Fixes: 430a67f9d616 ("block, bfq: merge bursts of newly-created queues")
    Tested-by: Holger Hoffstätte <holger@xxxxxxxxxxxxxxxxxxxxxx>
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-6-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit 7812472f973047a886e4ed9a91d98d6627dd746f
Author: Pietro Pedroni <pedroni.pietro.96@xxxxxxxxx>
Date:   Sat Jun 19 16:09:45 2021 +0200

    block, bfq: boost throughput by extending queue-merging times
    
    One of the methods with which bfq boosts throughput is by merging queues.
    One of the merging variants in bfq is the stable merge.
    This mechanism is activated between two queues only if they are created
    within a certain maximum time T1 from each other.
    Merging can happen soon or be delayed. In the second case, before
    merging, bfq needs to evaluate a throughput-boost parameter that
    indicates whether the queue generates a high throughput is served alone.
    Merging occurs when this throughput-boost is not high enough.
    In particular, this parameter is evaluated and late merging may occur
    only after at least a time T2 from the creation of the queue.
    
    Currently T1 and T2 are set to 180ms and 200ms, respectively.
    In this way the merging mechanism rarely occurs because time is not
    enough. This results in a noticeable lowering of the overall throughput
    with some workloads (see the example below).
    
    This commit introduces two constants bfq_activation_stable_merging and
    bfq_late_stable_merging in order to increase the duration of T1 and T2.
    Both the stable merging activation time and the late merging
    time are set to 600ms. This value has been experimentally evaluated
    using sqlite benchmark in the Phoronix Test Suite on a HDD.
    The duration of the benchmark before this fix was 111.02s, while now
    it has reached 97.02s, a better result than that of all the other
    schedulers.
    
    Signed-off-by: Pietro Pedroni <pedroni.pietro.96@xxxxxxxxx>
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-5-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit d4f49983fa3944416c28379c35fbe10c68455ea4
Author: Paolo Valente <paolo.valente@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:44 2021 +0200

    block, bfq: consider also creation time in delayed stable merge
    
    Since commit 430a67f9d616 ("block, bfq: merge bursts of newly-created
    queues"), BFQ may schedule a merge between a newly created sync
    bfq_queue and the last sync bfq_queue created. Such a merging is not
    performed immediately, because BFQ needs first to find out whether the
    newly created queue actually reaches a higher throughput if not merged
    at all (and in that case BFQ will not perform any stable merging). To
    check that, a little time must be waited after the creation of the new
    queue, so that some I/O can flow in the queue, and statistics on such
    I/O can be computed.
    
    Yet, to evaluate the above waiting time, the last split time is
    considered as start time, instead of the creation time of the
    queue. This is a mistake, because considering the split time is
    correct only in the following scenario.
    
    The queue undergoes a non-stable merges on the arrival of its very
    first I/O request, due to close I/O with some other queue. While the
    queue is merged for close I/O, stable merging is not considered. Yet
    the queue may then happen to be split, if the close I/O finishes (or
    happens to be a false positive). From this time on, the queue can
    again be considered for stable merging. But, again, a little time must
    elapse, to let some new I/O flow in the queue and to get updated
    statistics. To wait for this time, the split time is to be taken into
    account.
    
    Yet, if the queue does not undergo a non-stable merge on the arrival
    of its very first request, then BFQ immediately checks whether the
    stable merge is to be performed. It happens because the split time for
    a queue is initialized to minus infinity when the queue is created.
    
    This commit fixes this mistake by adding the missing condition. Now
    the check for delayed stable-merge is performed after a little time is
    elapsed not only from the last queue split time, but also from the
    creation time of the queue.
    
    Fixes: 430a67f9d616 ("block, bfq: merge bursts of newly-created queues")
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-4-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit e03f2ab78a4a673e4af23c3b855591c48b9de4d7
Author: Luca Mariotti <mariottiluca1@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:43 2021 +0200

    block, bfq: fix delayed stable merge check
    
    When attempting to schedule a merge of a given bfq_queue with the currently
    in-service bfq_queue or with a cooperating bfq_queue among the scheduled
    bfq_queues, delayed stable merge is checked for rotational or non-queueing
    devs. For this stable merge to be performed, some conditions must be met.
    If the current bfq_queue underwent some split from some merged bfq_queue,
    one of these conditions is that two hundred milliseconds must elapse from
    split, otherwise this condition is always met.
    
    Unfortunately, by mistake, time_is_after_jiffies() was written instead of
    time_is_before_jiffies() for this check, verifying that less than two
    hundred milliseconds have elapsed instead of verifying that at least two
    hundred milliseconds have elapsed.
    
    Fix this issue by replacing time_is_after_jiffies() with
    time_is_before_jiffies().
    
    Signed-off-by: Luca Mariotti <mariottiluca1@xxxxxxxxxx>
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Signed-off-by: Pietro Pedroni <pedroni.pietro.96@xxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-3-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

commit 511a2699237611b062df7798476bf3a1392910b9
Author: Paolo Valente <paolo.valente@xxxxxxxxxx>
Date:   Sat Jun 19 16:09:42 2021 +0200

    block, bfq: let also stably merged queues enjoy weight raising
    
    Merged bfq_queues are kept out of weight-raising (low-latency)
    mechanisms. The reason is that these queues are usually created for
    non-interactive and non-soft-real-time tasks. Yet this is not the case
    for stably-merged queues. These queues are merged just because they
    are created shortly after each other. So they may easily serve the I/O
    of an interactive or soft-real time application, if the application
    happens to spawn multiple processes.
    
    To address this issue, this commits lets also stably-merged queued
    enjoy weight raising.
    
    Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20210619140948.98712-2-paolo.valente@xxxxxxxxxx
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>