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

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

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

Once the 32-bit seq wraps, a newer bm_seq can look smaller
than old, so .. covert to wrap-safe calculate way.

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 6d01c35b5499..a08230aac711 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -997,11 +997,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
 			md_wakeup_thread(conf->mddev->thread);
 
 	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) &&
-		    sh->batch_head->bm_seq > seq)
+		    sh->batch_head->bm_seq - seq > 0)
 			seq = sh->batch_head->bm_seq;
 		set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
 		sh->batch_head->bm_seq = seq;
 	}
 
-- 
2.54.0

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

[sashiko-bot]

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 <chencheng@fnnas.com>

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)

[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;

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

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

[Chen Cheng]

在 2026/6/18 11:12, sashiko-bot@kernel.org 写道:
> 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 <chencheng@fnnas.com>
> 
> 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 .

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

[yu kuai]

在 2026/6/18 10:57, Chen Cheng 写道:

> From: Chen Cheng<chencheng@fnnas.com>
>
> Once the 32-bit seq wraps, a newer bm_seq can look smaller
> than old, so .. covert to wrap-safe calculate way.
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
>   drivers/md/raid5.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
Applied to md-7.2

-- 
Thanks,
Kuai