linux-kernel - Re: [PATCH] blk-mq: Fix stall due to recursive flush plug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZK6dpy7w/4YSwbBi@infradead.org>
Date:   Wed, 12 Jul 2023 05:33:43 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Ross Lagerwall <ross.lagerwall@...rix.com>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH] blk-mq: Fix stall due to recursive flush plug

On Tue, Jul 11, 2023 at 05:04:34PM +0100, Ross Lagerwall wrote:
> We have seen rare IO stalls as follows:
> 
> * blk_mq_plug_issue_direct() is entered with an mq_list containing two
> requests.
> * For the first request, it sets last == false and enters the driver's
> queue_rq callback.
> * The driver queue_rq callback indirectly calls schedule() which calls
> blk_flush_plug().

-> this assumes BLK_MQ_F_BLOCKING is set, as otherwise ->queue_rq can't
sleep.

> * blk_flush_plug() handles the remaining request in the mq_list. mq_list
> is now empty.
> * The original call to queue_rq resumes (with last == false).
> * The loop in blk_mq_plug_issue_direct() terminates because there are no
> remaining requests in mq_list.
> 
> The IO is now stalled because the last request submitted to the driver
> had last == false and there was no subsequent call to commit_rqs().
> 
> Fix this by returning early in blk_mq_flush_plug_list() if rq_count is 0
> which it will be in the recursive case, rather than checking if the
> mq_list is empty. At the same time, adjust one of the callers to skip
> the mq_list empty check as it is not necessary.

>From what I can tell this looks correct, but at the same time very
fragile.  At least we need a comment on learing plug->rq_count early
in blk_mq_flush_plug_list about this recursion potential, probably
paired with another one where we're checking rq_count instead of the
list now.  I wonder if there is a better way to do this in a more
explicit way, but I can't think of one right now.