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

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 <[email protected]>

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/[email protected]?part=1