Re: [PATCH v4 1/7] block: Prevent the use of REQ_FUA with read operations

[Christoph Hellwig <hch@infradead.org>]

On Wed, Nov 02, 2022 at 07:05:35AM +0900, Damien Le Moal wrote:
> >> +	if (!op_is_write(rq->cmd_flags) && (rq->cmd_flags & REQ_FUA)) {
> >> +		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
> > 
> > How could this even happen?  If we want a debug check,  I think it
> > should be in submit_bio and a WARN_ON_ONCE.
> 
> I have not found any code that issues a FUA read. So I do not think this
> can happen at all currently. The check is about making sure that it
> *never* happens.
> 
> I thought of having the check higher up in the submit path but I wanted to
> avoid adding yet another check in the very hot path. But if you are OK
> with that, I will move it.

I'd do something like this:

---
From 96847cce848938d1ee368e609ccb28a19854fba3 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 2 Nov 2022 08:05:41 +0100
Subject: block: add a sanity check for non-write flush/fua bios

Check that the PREFUSH and FUA flags are only set on write bios,
given that the flush state machine expects that.

Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index e9e2bf15cd909..4e2b01a53c6ab 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -720,12 +720,15 @@ void submit_bio_noacct(struct bio *bio)
 	 * Filter flush bio's early so that bio based drivers without flush
 	 * support don't have to worry about them.
 	 */
-	if (op_is_flush(bio->bi_opf) &&
-	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
-		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
-		if (!bio_sectors(bio)) {
-			status = BLK_STS_OK;
+	if (op_is_flush(bio->bi_opf)) {
+		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE))
 			goto end_io;
+		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
+			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
+			if (!bio_sectors(bio)) {
+				status = BLK_STS_OK;
+				goto end_io;
+			}
 		}
 	}
 
-- 
2.30.2