Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe

在 2026/6/18 11:12, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Wrap-safe sequence comparison introduces signed integer overflow, triggering UBSAN.
> 
> Pre-existing issues:
> - [High] Concurrent lockless read-modify-write of `batch_head->bm_seq` can result in lost bitmap flush sequence updates.
> --
> 
> commit 38cbf25fbeef1204525d320e539132e2704c3653
> Author: Chen Cheng <[email protected]>
> 
> md/raid5: let stripe batch bm_seq comparison wrap-safe
> 
> This commit alters the comparison of sequence numbers in the RAID5
> stripe batching path to correctly handle wrap-around by using subtraction.
> 
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d0d7f96fd7cd9..5882670f7d0c9 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -996,7 +996,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
>>   	if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
>>   		int seq = sh->bm_seq;
>>   		if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but it appears that the shared
> batch_head->bm_seq is being accessed and updated locklessly here. If multiple
> threads concurrently append stripes to different parts of the batch list,
> could Thread A and Thread B concurrently read batch_head->bm_seq, compare it,
> and write it back, causing a race?
> 
> An unguarded read-modify-write might allow a thread writing an older
> sequence number to overwrite a newer one written by another thread. If bm_seq
> gets overwritten with an older value, could the batch start processing before
> the newer bitmap flush completes, potentially risking data corruption on power
> loss?
> 
>> -		    sh->batch_head->bm_seq > seq)
>> +		    sh->batch_head->bm_seq - seq > 0)
> 

good catch..

> [Severity: Medium]
> Since both sh->batch_head->bm_seq and seq are signed ints, does this wrap-safe
> comparison introduce a signed integer overflow when the sequence wraps?
> 
> While the kernel is compiled with -fno-strict-overflow so it functions as
> intended, subtracting two signed integers whose difference exceeds the
> representable range of a 32-bit signed integer can trigger Undefined Behavior
> Sanitizer (UBSAN) splats.
> 
> Could this be written by casting to unsigned before subtraction and back to
> signed, such as (s32)((u32)a - (u32)b) > 0, to avoid UBSAN warnings and
> potential kernel panics on environments with panic_on_warn?
> 
>>   			seq = sh->batch_head->bm_seq;
>>   		set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
>>   		sh->batch_head->bm_seq = seq;
> 

I am fine with both way .. raid5 use the template "a - b > 0" directly 
for a long time

     e.g. "sh->bm_seq - conf->seq_write > 0" in do_release_stripe()

(s32)((u32)a - (u32)b) > 0 is more rigorous, and "a - b > 0" much cleaner .