linux-kernel - Re: [PATCH] block: fix request.queuelist usage in flush

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2223bbb9-8bc8-4566-9c3f-ef6103422068@linux.dev>
Date: Fri, 7 Jun 2024 14:24:52 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Christoph Hellwig <hch@....de>
Cc: Friedrich Weber <f.weber@...xmox.com>, axboe@...nel.dk,
 ming.lei@...hat.com, bvanassche@....org, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, zhouchengming@...edance.com,
 Thomas Lamprecht <t.lamprecht@...xmox.com>
Subject: Re: [PATCH] block: fix request.queuelist usage in flush

On 2024/6/7 12:55, Christoph Hellwig wrote:
> On Fri, Jun 07, 2024 at 10:37:58AM +0800, Chengming Zhou wrote:
>> Yeah, right, it seems LVM may create this special request that only has
>> PREFLUSH | POSTFLUSH without any DATA, goes into the flush state machine.
>> Then, cause the request double list_add_tail() without list_del_init().
>> I don't know the reason behind it, but well, it's allowable in the current
>> flush code.
> 
> PREFLUSH | POSTFLUSH is a weird invalid format.  We'll need to fix this
> in dm, and probably also catch it in the block layer submission path.
> 

Right, how about add WARN here to catch it? Or just set it to PREFLUSH?
Not familiar with dm code, need help if we need to fix it in dm. :)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index c17cf8ed8113..3ce9ed78c375 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -185,7 +185,7 @@ static void blk_flush_complete_seq(struct request *rq,
                /* queue for flush */
                if (list_empty(pending))
                        fq->flush_pending_since = jiffies;
-               list_move_tail(&rq->queuelist, pending);
+               list_add_tail(&rq->queuelist, pending);
                break;

        case REQ_FSEQ_DATA:
@@ -263,6 +263,7 @@ static enum rq_end_io_ret flush_end_io(struct request *flush_rq,
                unsigned int seq = blk_flush_cur_seq(rq);

                BUG_ON(seq != REQ_FSEQ_PREFLUSH && seq != REQ_FSEQ_POSTFLUSH);
+               list_del_init(&rq->queuelist);
                blk_flush_complete_seq(rq, fq, seq, error);
        }

@@ -402,6 +403,12 @@ bool blk_insert_flush(struct request *rq)
        unsigned int policy = blk_flush_policy(fflags, rq);
        struct blk_flush_queue *fq = blk_get_flush_queue(q, rq->mq_ctx);

+       /*
+        * PREFLUSH | POSTFLUSH is a weird invalid format,
+        * need to fix in the upper layer, catch it here.
+        */
+       WARN_ON_ONCE(policy == (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH));
+
        /* FLUSH/FUA request must never be merged */
        WARN_ON_ONCE(rq->bio != rq->biotail);