• Douglas Anderson's avatar
    block, bfq: NULL out the bic when it's no longer valid · 340a4da3
    Douglas Anderson authored
    commit dbc3117d4ca9e17819ac73501e914b8422686750 upstream.
    In reboot tests on several devices we were seeing a "use after free"
    when slub_debug or KASAN was enabled.  The kernel complained about:
      Unable to handle kernel paging request at virtual address 6b6b6c2b
    ...which is a classic sign of use after free under slub_debug.  The
    stack crawl in kgdb looked like:
     0  test_bit (addr=<optimized out>, nr=<optimized out>)
     1  bfq_bfqq_busy (bfqq=<optimized out>)
     2  bfq_select_queue (bfqd=<optimized out>)
     3  __bfq_dispatch_request (hctx=<optimized out>)
     4  bfq_dispatch_request (hctx=<optimized out>)
     5  0xc056ef00 in blk_mq_do_dispatch_sched (hctx=0xed249440)
     6  0xc056f728 in blk_mq_sched_dispatch_requests (hctx=0xed249440)
     7  0xc0568d24 in __blk_mq_run_hw_queue (hctx=0xed249440)
     8  0xc0568d94 in blk_mq_run_work_fn (work=<optimized out>)
     9  0xc024c5c4 in process_one_work (worker=0xec6d4640, work=0xed249480)
     10 0xc024cff4 in worker_thread (__worker=0xec6d4640)
    Digging in kgdb, it could be found that, though bfqq looked fine,
    bfqq->bic had been freed.
    Through further digging, I postulated that perhaps it is illegal to
    access a "bic" (AKA an "icq") after bfq_exit_icq() had been called
    because the "bic" can be freed at some point in time after this call
    is made.  I confirmed that there certainly were cases where the exact
    crashing code path would access the "bic" after bfq_exit_icq() had
    been called.  Sspecifically I set the "bfqq->bic" to (void *)0x7 and
    saw that the bic was 0x7 at the time of the crash.
    To understand a bit more about why this crash was fairly uncommon (I
    saw it only once in a few hundred reboots), you can see that much of
    the time bfq_exit_icq_fbqq() fully frees the bfqq and thus it can't
    access the ->bic anymore.  The only case it doesn't is if
    bfq_put_queue() sees a reference still held.
    However, even in the case when bfqq isn't freed, the crash is still
    rare.  Why?  I tracked what happened to the "bic" after the exit
    routine.  It doesn't get freed right away.  Rather,
    put_io_context_active() eventually called put_io_context() which
    queued up freeing on a workqueue.  The freeing then actually happened
    later than that through call_rcu().  Despite all these delays, some
    extra debugging showed that all the hoops could be jumped through in
    time and the memory could be freed causing the original crash.  Phew!
    To make a long story short, assuming it truly is illegal to access an
    icq after the "exit_icq" callback is finished, this patch is needed.
    Cc: stable@vger.kernel.org
    Reviewed-by: default avatarPaolo Valente <paolo.valente@unimore.it>
    Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
    Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>