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

["Chen Cheng" <chencheng@fnnas.com>]

在 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 .