Received-SPF: pass (google.com: domain of linux-kernel+bounces-175882-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Breno Leitao <leitao@debian.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: paulmck@kernel.org,
	linux-block@vger.kernel.org (open list:BLOCK LAYER),
	linux-kernel@vger.kernel.org (open list)
Subject: [PATCH] block: Annotate a racy read in blk_do_io_stat()
Date: Fri, 10 May 2024 07:19:18 -0700
Message-ID: <20240510141921.883231-1-leitao@debian.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

KCSAN has reported a potential data race in blk_mq subsystem where
reading the rq->flag.

	BUG: KCSAN: data-race in __blk_mq_end_request / blk_mq_check_inflight

	read-write to 0xffff888120514d1c of 4 bytes by interrupt on cpu 6:
	__blk_mq_end_request (block/blk-mq.c:700 block/blk-mq.c:1040)
	scsi_end_request (drivers/scsi/scsi_lib.c:667)
	scsi_io_completion (drivers/scsi/scsi_lib.c:1068)
	scsi_finish_command (drivers/scsi/scsi.c:199)
	scsi_complete (drivers/scsi/scsi_lib.c:?)
	blk_done_softirq (block/blk-mq.c:? block/blk-mq.c:1134)
	handle_softirqs (./arch/x86/include/asm/jump_label.h:27
			 ./include/linux/jump_label.h:207
			 ./include/trace/events/irq.h:142 kernel/softirq.c:555)
	__irq_exit_rcu (kernel/softirq.c:617 kernel/softirq.c:639)
	irq_exit_rcu (kernel/softirq.c:651)
	common_interrupt (arch/x86/kernel/irq.c:247)
	asm_common_interrupt (./arch/x86/include/asm/idtentry.h:693)
	cpuidle_enter_state (drivers/cpuidle/cpuidle.c:291)
	cpuidle_enter (drivers/cpuidle/cpuidle.c:388)
	do_idle (kernel/sched/idle.c:155 kernel/sched/idle.c:236
		 kernel/sched/idle.c:332)
	cpu_startup_entry (kernel/sched/idle.c:429)
	start_secondary (arch/x86/kernel/smpboot.c:313)
	common_startup_64 (arch/x86/kernel/head_64.S:421)

	read to 0xffff888120514d1c of 4 bytes by task 9106 on cpu 51:
	blk_mq_check_inflight (block/blk.h:356 block/blk-mq.c:94)
	14:06:18 bt_iter (block/blk-mq-tag.c:292)
	sbitmap_for_each_set (./include/linux/sbitmap.h:284
			 ./include/linux/sbitmap.h:302)
	blk_mq_queue_tag_busy_iter (block/blk-mq-tag.c:? block/blk-mq-tag.c:533)
	blk_mq_in_flight (block/blk-mq.c:109)
	diskstats_show (block/genhd.c:?)
	seq_read_iter (fs/seq_file.c:?)
	proc_reg_read_iter (fs/proc/inode.c:299)
	vfs_read (fs/read_write.c:396 fs/read_write.c:476)
	ksys_read (fs/read_write.c:619)
	__x64_sys_read (fs/read_write.c:627)
	x64_sys_call (arch/x86/entry/syscall_64.c:33)
	do_syscall_64 (arch/x86/entry/common.c:?)
	entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)

	value changed: 0x00022382 -> 0x00022182

Discussing it with Jens Axboe and Pavel Begunkov, they suggested we just
want to annotated this with data_race(), since disk statistic reading
isn't critical, and it will not be a big deal if this bit is not stable.

Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 block/blk.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk.h b/block/blk.h
index d9f584984bc4..57a1d73a0718 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -353,7 +353,8 @@ int blk_dev_init(void);
  */
 static inline bool blk_do_io_stat(struct request *rq)
 {
-	return (rq->rq_flags & RQF_IO_STAT) && !blk_rq_is_passthrough(rq);
+	/* Disk stats reading isn’t critical, let it race */
+	return (data_race(rq->rq_flags) & RQF_IO_STAT) && !blk_rq_is_passthrough(rq);
 }
 
 void update_io_ticks(struct block_device *part, unsigned long now, bool end);
-- 
2.43.0