[PATCH] md/raid1: protect head_position for read balance

[PATCH] md/raid1: protect head_position for read balance

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

KCSAN reports a data race between raid1_end_read_request() and
raid1_read_request().

The completion path updates conf->mirrors[disk].head_position in
update_head_pos() without a lock, while the read-balance heuristic reads
the same field locklessly in is_sequential() and choose_best_rdev().

KCSAN report:
=========================
  BUG: KCSAN: data-race in raid1_end_read_request / raid1_read_request

  write to 0xffff8f0306ba7868 of 8 bytes by interrupt on cpu 9:
    raid1_end_read_request+0xb5/0x440
    bio_endio+0x3c9/0x3e0
    blk_update_request+0x257/0x770
    scsi_end_request+0x4d/0x520
    scsi_io_completion+0x6f/0x990
    scsi_finish_command+0x188/0x280
    scsi_complete+0xac/0x160
    blk_complete_reqs+0x8e/0xb0
    blk_done_softirq+0x1d/0x30
   [...]

  read to 0xffff8f0306ba7868 of 8 bytes by task 667002 on cpu 11:
    raid1_read_request+0x497/0x1a10
    raid1_make_request+0xdf/0x1950
    md_handle_request+0x2c5/0x700
    md_submit_bio+0x126/0x320
    __submit_bio+0x2ec/0x3a0
    submit_bio_noacct_nocheck+0x572/0x890
   [...]

  value changed: 0x0000000000000078 -> 0x00000000005fe448

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 5b9368bd9e70..4cdf4484cab6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -357,12 +357,12 @@ static void raid_end_bio_io(struct r1bio *r1_bio)
  */
 static inline void update_head_pos(int disk, struct r1bio *r1_bio)
 {
 	struct r1conf *conf = r1_bio->mddev->private;
 
-	conf->mirrors[disk].head_position =
-		r1_bio->sector + (r1_bio->sectors);
+	WRITE_ONCE(conf->mirrors[disk].head_position,
+		   r1_bio->sector + r1_bio->sectors);
 }
 
 /*
  * Find the disk number which triggered given bio
  */
@@ -735,11 +735,11 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
 
 static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
 {
 	/* TODO: address issues with this check and concurrency. */
 	return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
-	       conf->mirrors[disk].head_position == r1_bio->sector;
+	       READ_ONCE(conf->mirrors[disk].head_position) == r1_bio->sector;
 }
 
 /*
  * If buffered sequential IO size exceeds optimal iosize, check if there is idle
  * disk. If yes, choose the idle disk.
@@ -812,11 +812,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 		/* At least two disks to choose from so failfast is OK */
 		if (ctl.readable_disks++ == 1)
 			set_bit(R1BIO_FailFast, &r1_bio->state);
 
 		pending = atomic_read(&rdev->nr_pending);
-		dist = abs(r1_bio->sector - conf->mirrors[disk].head_position);
+		dist = abs(r1_bio->sector -
+			   READ_ONCE(conf->mirrors[disk].head_position));
 
 		/* Don't change to another disk for sequential reads */
 		if (is_sequential(conf, disk, r1_bio)) {
 			if (!should_choose_next(conf, disk))
 				return disk;
-- 
2.54.0

Re: [PATCH] md/raid1: protect head_position for read balance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The commit fixes a KCSAN-reported data race on `head_position` in `raid1.c` but misses the identical data race in the sibling `raid10.c` subsystem.
- [Medium] The patch addresses a KCSAN data race for `head_position` but misses identical concurrent data races on adjacent heuristic variables `next_seq_sect` and `seq_start`, leading to a potential double-fetch bug and unsigned integer underflow.
--

commit 754549019263d81271c296e50e19909fb366125b
Author: Chen Cheng <chencheng@fnnas.com>

md/raid1: protect head_position for read balance

KCSAN reports a data race between raid1_end_read_request() and
raid1_read_request().

The completion path updates conf->mirrors[disk].head_position in
update_head_pos() without a lock, while the read-balance heuristic reads
the same field locklessly in is_sequential() and choose_best_rdev().

[Severity: Medium]
This is a pre-existing issue, but does raid10 also need this same fix?

Looking at drivers/md/raid10.c, update_head_pos() updates
conf->mirrors[r10_bio->devs[slot].devnum].head_position locklessly, and
read_balance() reads it locklessly:

drivers/md/raid10.c:read_balance() {
...
	new_distance = abs(r10_bio->devs[slot].addr -
			   conf->mirrors[disk].head_position);
...
}

Since raid10 shares this read-balancing heuristic, should the data race be
addressed there as well to prevent torn reads?

[ ... ]

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 5b9368bd9e700..4cdf4484cab6c 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -737,7 +737,7 @@ static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
>  {
>  	/* TODO: address issues with this check and concurrency. */
>  	return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
> -	       conf->mirrors[disk].head_position == r1_bio->sector;
> +	       READ_ONCE(conf->mirrors[disk].head_position) == r1_bio->sector;
>  }

[Severity: Medium]
This is a pre-existing issue, but should next_seq_sect and seq_start also
be protected to prevent double-fetch bugs and underflows?

While this patch secures head_position, next_seq_sect is still read locklessly
here and in should_choose_next():

drivers/md/raid1.c:should_choose_next() {
...
	return opt_iosize > 0 && mirror->seq_start != MaxSector &&
	       mirror->next_seq_sect > opt_iosize &&
	       mirror->next_seq_sect - opt_iosize >= mirror->seq_start;
}

Because mirror->next_seq_sect is read multiple times without READ_ONCE(),
could the compiler emit multiple memory loads?

If a concurrent update from update_read_sectors() reduces next_seq_sect
after the safety check (mirror->next_seq_sect > opt_iosize) but before the
subtraction, could it cause an unsigned integer underflow, bypassing the
seq_start check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619044114.1208456-1-chencheng@fnnas.com?part=1

Re: [PATCH] md/raid1: protect head_position for read balance

[Chen Cheng]

在 2026/6/19 12:51, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Medium] The commit fixes a KCSAN-reported data race on `head_position` in `raid1.c` but misses the identical data race in the sibling `raid10.c` subsystem.
> - [Medium] The patch addresses a KCSAN data race for `head_position` but misses identical concurrent data races on adjacent heuristic variables `next_seq_sect` and `seq_start`, leading to a potential double-fetch bug and unsigned integer underflow.
> --
> 
> commit 754549019263d81271c296e50e19909fb366125b
> Author: Chen Cheng <chencheng@fnnas.com>
> 
> md/raid1: protect head_position for read balance
> 
> KCSAN reports a data race between raid1_end_read_request() and
> raid1_read_request().
> 
> The completion path updates conf->mirrors[disk].head_position in
> update_head_pos() without a lock, while the read-balance heuristic reads
> the same field locklessly in is_sequential() and choose_best_rdev().
> 
> [Severity: Medium]
> This is a pre-existing issue, but does raid10 also need this same fix?
> 
> Looking at drivers/md/raid10.c, update_head_pos() updates
> conf->mirrors[r10_bio->devs[slot].devnum].head_position locklessly, and
> read_balance() reads it locklessly:
> 
> drivers/md/raid10.c:read_balance() {
> ...
> 	new_distance = abs(r10_bio->devs[slot].addr -
> 			   conf->mirrors[disk].head_position);
> ...
> }
> 
> Since raid10 shares this read-balancing heuristic, should the data race be
> addressed there as well to prevent torn reads?
> 
> [ ... ]
> 

sure, already submited in another patch .


>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 5b9368bd9e700..4cdf4484cab6c 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
> [ ... ]
>> @@ -737,7 +737,7 @@ static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
>>   {
>>   	/* TODO: address issues with this check and concurrency. */
>>   	return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
>> -	       conf->mirrors[disk].head_position == r1_bio->sector;
>> +	       READ_ONCE(conf->mirrors[disk].head_position) == r1_bio->sector;
>>   }
> 
> [Severity: Medium]
> This is a pre-existing issue, but should next_seq_sect and seq_start also
> be protected to prevent double-fetch bugs and underflows?
> 
> While this patch secures head_position, next_seq_sect is still read locklessly
> here and in should_choose_next():
> 
> drivers/md/raid1.c:should_choose_next() {
> ...
> 	return opt_iosize > 0 && mirror->seq_start != MaxSector &&
> 	       mirror->next_seq_sect > opt_iosize &&
> 	       mirror->next_seq_sect - opt_iosize >= mirror->seq_start;
> }
> 
> Because mirror->next_seq_sect is read multiple times without READ_ONCE(),
> could the compiler emit multiple memory loads?
> 
> If a concurrent update from update_read_sectors() reduces next_seq_sect
> after the safety check (mirror->next_seq_sect > opt_iosize) but before the
> subtraction, could it cause an unsigned integer underflow, bypassing the
> seq_start check?
> 

good catch , and , I notice either. I will submit in separate patch .