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

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)

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