linux-kernel - [patch] blk-flush: fix flush policy calculation

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

Message-ID: <x49d3go98ku.fsf@segfault.boston.devel.redhat.com>
Date:	Mon, 01 Aug 2011 16:32:17 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	linux-kernel@...r.kernel.org
Cc:	Tejun Heo <tj@...nle.org>, Jens Axboe <jaxboe@...ionio.com>
Subject: [patch] blk-flush: fix flush policy calculation

Hi,

Reading through the code in blk-flush.c, it appears that there is an
oversight in the policy returned from blk_flush_policy:

        if (fflags & REQ_FLUSH) {
                if (rq->cmd_flags & REQ_FLUSH)
                        policy |= REQ_FSEQ_PREFLUSH;
                if (blk_rq_sectors(rq))
                        policy |= REQ_FSEQ_DATA;
                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
                        policy |= REQ_FSEQ_POSTFLUSH;
        }
        return policy;

This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
include FLUSH and/or FUA.  However, the short-circuit for not issuing
flushes when the device doesn't need/support them depends on
REQ_FSEQ_DATA being set while the other two bits are clear:

        /*
         * If there's data but flush is not necessary, the request can be
         * processed directly without going through flush machinery.  Queue
         * for normal execution.
         */
        if ((policy & REQ_FSEQ_DATA) &&
            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
                list_add_tail(&rq->queuelist, &q->queue_head);
                return;
        }

Given the code as it stands, I don't think the body of this if statement
will ever be executed.  I've attached a fix for this below.  It seems
like this could be both a performance and a correctness issue, though
I've not run into any problems I can directly attribute to this (perhaps
due to file systems not issuing flushes when support is not advertised?).

Comments are appreciated.

Cheers,
Jeff

Signed-off-by: Jeff Moyer <jmoyer@...hat.com>

diff --git a/block/blk-flush.c b/block/blk-flush.c
index bb21e4c..3a06118 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
 {
 	unsigned int policy = 0;
 
+	if (blk_rq_sectors(rq))
+		policy |= REQ_FSEQ_DATA;
 	if (fflags & REQ_FLUSH) {
 		if (rq->cmd_flags & REQ_FLUSH)
 			policy |= REQ_FSEQ_PREFLUSH;
-		if (blk_rq_sectors(rq))
-			policy |= REQ_FSEQ_DATA;
 		if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
 			policy |= REQ_FSEQ_POSTFLUSH;
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/