Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Direct integer comparison of sequence numbers `bm_seq` in `stripe_add_to_batch_list` fails to handle integer wrap-around.
--

commit d138461da7f53106a83faa762ae81dbb822cbfca
Author: Chen Cheng <[email protected]>

md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency

This patch protects seq_flush and seq_write consistency across multiple CPUs.
It uses READ_ONCE() and WRITE_ONCE() to prevent data races detected by KCSAN
when assigning sh->bm_seq without holding device_lock.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d0d7f96fd7cd9..891c2cb605d30 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -3538,7 +3538,7 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
>  		 sh->dev[dd_idx].sector);
>  
>  	if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
> -		sh->bm_seq = conf->seq_flush+1;
> +		sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
>  		set_bit(STRIPE_BIT_DELAY, &sh->state);
>  	}
>  }

[Severity: Critical]
This is a pre-existing issue, but does stripe_add_to_batch_list() handle
32-bit integer wrap-around correctly when comparing sequence numbers?

Since seq_flush, seq_write, and sh->bm_seq increment for each batch, they
will eventually overflow and wrap around to negative values. In other parts
of the code, this is handled using signed subtraction (e.g.,
sh->bm_seq - conf->seq_write > 0).

However, in stripe_add_to_batch_list(), there is a direct comparison:

drivers/md/raid5.c:stripe_add_to_batch_list() {
    ...
    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)
            seq = sh->batch_head->bm_seq;
    ...
}

At the wrap-around boundary, a newer sequence number is numerically less than
an older sequence number. Does this direct comparison incorrectly evaluate to
false, assigning the batch head an older sequence number?

Could this cause the batch to be released early when conf->seq_write catches
up, completing before its newer bitmap updates are fully flushed? Should this
comparison use subtraction instead, such as sh->batch_head->bm_seq - seq > 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1