* [bug report] raid5-ppl: PPL support for disks with write-back cache enabled
From: Dan Carpenter @ 2026-06-17 6:59 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
This code is nine years old, so what I like to do is add it to the KTODO
in case anyone wants to fix it.
KTODO: Fix use after free in ppl_do_flush()
Hello Tomasz Majchrzak,
Commit 1532d9e87e8b ("raid5-ppl: PPL support for disks with
write-back cache enabled") from Dec 27, 2017 (linux-next), leads to
the following Smatch static checker warning:
drivers/md/raid5-ppl.c:646 ppl_do_flush()
warn: 'io' was already freed. (line 647)
drivers/md/raid5-ppl.c
608 static void ppl_do_flush(struct ppl_io_unit *io)
609 {
610 struct ppl_log *log = io->log;
611 struct ppl_conf *ppl_conf = log->ppl_conf;
612 struct r5conf *conf = ppl_conf->mddev->private;
613 int raid_disks = conf->raid_disks;
614 int flushed_disks = 0;
615 int i;
616
617 atomic_set(&io->pending_flushes, raid_disks);
618
619 for_each_set_bit(i, &log->disk_flush_bitmap, raid_disks) {
620 struct md_rdev *rdev;
621 struct block_device *bdev = NULL;
622
623 rdev = conf->disks[i].rdev;
624 if (rdev && !test_bit(Faulty, &rdev->flags))
625 bdev = rdev->bdev;
626
627 if (bdev) {
628 struct bio *bio;
629
630 bio = bio_alloc_bioset(bdev, 0,
631 REQ_OP_WRITE | REQ_PREFLUSH,
632 GFP_NOIO, &ppl_conf->flush_bs);
633 bio->bi_private = io;
634 bio->bi_end_io = ppl_flush_endio;
635
636 pr_debug("%s: dev: %ps\n", __func__, bio->bi_bdev);
637
638 submit_bio(bio);
639 flushed_disks++;
640 }
641 }
642
643 log->disk_flush_bitmap = 0;
644
645 for (i = flushed_disks ; i < raid_disks; i++) {
--> 646 if (atomic_dec_and_test(&io->pending_flushes))
647 ppl_io_unit_finished(io);
The ppl_io_unit_finished() function frees "io" so probably there is
supposed to be a statement after it. The atomic_dec_and_test() will
underflow on subsequent iterations through the loop which is normally
harmless. We may want to convert this to refcount_t so that any
underflows cause a WARN().
648 }
649 }
This email is a free service from the Smatch-CI project [smatch.sf.net].
regards,
dan carpenter
^ permalink raw reply
* Re: [bug report] raid5-ppl: PPL support for disks with write-back cache enabled
From: Dan Carpenter @ 2026-06-17 7:04 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
In-Reply-To: <ajJF2wKYWRk4GGCK@stanley.mountain>
On Wed, Jun 17, 2026 at 09:59:39AM +0300, Dan Carpenter wrote:
> This code is nine years old, so what I like to do is add it to the KTODO
> in case anyone wants to fix it.
>
> KTODO: Fix use after free in ppl_do_flush()
>
> Hello Tomasz Majchrzak,
>
> Commit 1532d9e87e8b ("raid5-ppl: PPL support for disks with
> write-back cache enabled") from Dec 27, 2017 (linux-next), leads to
> the following Smatch static checker warning:
>
> drivers/md/raid5-ppl.c:646 ppl_do_flush()
> warn: 'io' was already freed. (line 647)
>
> drivers/md/raid5-ppl.c
> 608 static void ppl_do_flush(struct ppl_io_unit *io)
> 609 {
> 610 struct ppl_log *log = io->log;
> 611 struct ppl_conf *ppl_conf = log->ppl_conf;
> 612 struct r5conf *conf = ppl_conf->mddev->private;
> 613 int raid_disks = conf->raid_disks;
> 614 int flushed_disks = 0;
> 615 int i;
> 616
> 617 atomic_set(&io->pending_flushes, raid_disks);
> 618
> 619 for_each_set_bit(i, &log->disk_flush_bitmap, raid_disks) {
> 620 struct md_rdev *rdev;
> 621 struct block_device *bdev = NULL;
> 622
> 623 rdev = conf->disks[i].rdev;
> 624 if (rdev && !test_bit(Faulty, &rdev->flags))
> 625 bdev = rdev->bdev;
> 626
> 627 if (bdev) {
> 628 struct bio *bio;
> 629
> 630 bio = bio_alloc_bioset(bdev, 0,
> 631 REQ_OP_WRITE | REQ_PREFLUSH,
> 632 GFP_NOIO, &ppl_conf->flush_bs);
> 633 bio->bi_private = io;
> 634 bio->bi_end_io = ppl_flush_endio;
> 635
> 636 pr_debug("%s: dev: %ps\n", __func__, bio->bi_bdev);
> 637
> 638 submit_bio(bio);
> 639 flushed_disks++;
> 640 }
> 641 }
> 642
> 643 log->disk_flush_bitmap = 0;
> 644
> 645 for (i = flushed_disks ; i < raid_disks; i++) {
> --> 646 if (atomic_dec_and_test(&io->pending_flushes))
> 647 ppl_io_unit_finished(io);
>
> The ppl_io_unit_finished() function frees "io" so probably there is
> supposed to be a statement after it.
This sentence a word missing. Probably there is supposed to be a *break*
statement.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-17 10:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Eric Biggers, Andrew Morton, linux-kernel, linux-crypto, x86,
linux-raid
In-Reply-To: <20260617055653.GB19218@lst.de>
On Wed, 17 Jun 2026 07:56:53 +0200
Christoph Hellwig <hch@lst.de> wrote:
> Can use the xor: prefix used for all other commits to lib/raid/xor?
>
> > Benchmark on AMD Ryzen 9 9950X (Zen 5):
> >
> > src_cnt avx avx512 Improvement
> > ======= ========== ========== ===========
> > 1 56353 MB/s 75388 MB/s 33%
> > 2 54274 MB/s 68409 MB/s 26%
> > 3 44649 MB/s 64042 MB/s 43%
> > 4 41315 MB/s 55002 MB/s 33%
>
> On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing
> AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not
> sure if is just the different benchmarking or something else going on.
I'd expect benchmarking of the xor loop to show that it is doing
2 memory reads every clock.
At 5GHz (I'm sure my zen5 will reach that single threaded) that
is 320 GB/s for src_cnt == 1 and 160 GB/s for src_cnt == 3.
200 GB/s with 32 byte cache reads would need a 200/32 = 6GHz cpu
just for the reads.
But I expect Eric is benchmarking more code and may be limited
by data cache refills.
>
> FYI, one or 2 sources are basically useless as they RAID5 configs
> that have no benefits over simple mirroring and thus the numbers
> aren't too interesting.
With three disks you xor two buffers (src_count == 1) to get the parity
to write to the third - so that is a valid RAID5 config.
>
> > +DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
> > + xor_avx512_5);
>
> Is there really much of a benefit of doing the historic DO_XOR_BLOCKS
> vs doing the loop manually? Especially as the common cases for a
> modern RAID will usually loop over more disks than this was built
> for. I.e., in practice one or two source buffers only happen at the
> end of a loop over more disks.
I stopped looking at what was being tested at that point :-)
David
^ permalink raw reply
* Re: [PATCH] md/raid5: protect bitmap batch counters aka seq_flush/seq_write
From: Chen Cheng @ 2026-06-17 13:55 UTC (permalink / raw)
To: sashiko-reviews, Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260616133034.015501F000E9@smtp.kernel.org>
在 2026/6/16 21:30, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing full memory barriers between the bitmap update/scan and `seq_flush` access allows torn writes.
> --
>
> commit 45ef15a300eccd14518bc90e254146a73a49bf51
> Author: Chen Cheng <chencheng@fnnas.com>
>
> md/raid5: protect bitmap batch counters aka seq_flush/seq_write
>
> This commit attempts to fix a KCSAN data-race report on conf->seq_flush
> in the raid5 code by introducing READ_ONCE() and WRITE_ONCE().
>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d0d7f96fd7cd9..ffc89ebbc4857 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;
> [Severity: Critical]
> This is a pre-existing issue, but does this lockless synchronization require
> a full memory barrier (smp_mb()) before reading seq_flush?
>
> The producer sequence updates the in-memory bitmap (which drops a lock,
> providing only a Store-Release barrier) and then reads seq_flush. Because
> there is no full memory barrier between the store to the bitmap and the load
> of seq_flush, weakly-ordered CPUs (and even x86, which allows Store-Load
> reordering) can reorder the read of seq_flush before the store to the
> bitmap becomes globally visible.
>
> Could this lead to silent data corruption on a crash if the consumer sees
> the old bitmap state but the producer assigns the stripe to the current
> batch?
barrier provide a tool to promise "the things before this statement all
done", and
I don't think I need such strong promise.
What I need is, "all cpu cache coherence for accesses variable
seq_flush", so
READ_ONCE/WRITE_ONCE is suitable for this scenario.
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>> }
> [ ... ]
>
>> @@ -5755,7 +5755,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
>> }
>> spin_unlock_irq(&sh->stripe_lock);
>> if (conf->mddev->bitmap) {
>> - sh->bm_seq = conf->seq_flush + 1;
>> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> [Severity: Critical]
> This is a pre-existing issue, but the same missing full memory barrier
> pattern appears here in the discard path as well.
>
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>>
> [ ... ]
>
>> @@ -6805,12 +6805,13 @@ static void raid5d(struct md_thread *thread)
>> if (
>> !list_empty(&conf->bitmap_list)) {
>> /* Now is a good time to flush some bitmap updates */
>> - conf->seq_flush++;
>> + int seq = READ_ONCE(conf->seq_flush) + 1;
>> + WRITE_ONCE(conf->seq_flush, seq);
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does the consumer side
> also rely dangerously on implicit barriers?
>
> The consumer (raid5d) writes seq_flush and relies on an implicit full
> barrier deep inside unplug() (via test_and_clear_bit) to order the
> subsequent bitmap scan.
>
> If the producer's missing barrier allows the read of seq_flush to be
> reordered, the consumer might flush the old batch and advance seq_write,
> unblocking the producer's stripe to be written to disk without its
> write-intent bitmap bit being flushed.
>
>> spin_unlock_irq(&conf->device_lock);
>> if (md_bitmap_enabled(mddev, true))
>> mddev->bitmap_ops->unplug(mddev, true);
>> spin_lock_irq(&conf->device_lock);
>> - conf->seq_write = conf->seq_flush;
>> + WRITE_ONCE(conf->seq_write, seq);
>> activate_bit_delay(conf, conf->temp_inactive_list);
>> }
>> raid5_activate_delayed(conf);
^ permalink raw reply
* [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: Chen Cheng @ 2026-06-17 14:28 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
kcsan detect race :
- raid5d() closes the current bitmap batch by updating
conf->seq_flush under conf->device_lock.
- __add_stripe_bio() read conf->seq_flush without that
lock when assigning sh->bm_seq.
so, protect seq_flush/seq_write consistency for multiple CPUs by
READ_ONCE()/WRITE_ONCE() under the path without held device_lock.
re-explain the stripe batch sequence number update flow:
1. sh->bm_seq declare which batch number the stripe belongs to
when perform bitmap-related write.
==> bm_seq = seq_flush+1
2. stripe be handled,
* if sh->bm_seq - conf->seq_write > 0, means the
batch stripes **newer than** the last written
batch, it cannot proceed yet, queued on bitmap_list.
* otherwise , has already proceed.
3. raid5d() `++seq_flush` to closes the current batch, means
* no more stripes join that old batch
* just-closed batch ready to write-out to disk
4. raid5d() calls bitmap hooks unplug() or writeout, then,
`++seq_write` to the same as bm_seq.
- seq_flush - for producer, to close batches.
- seq_write - for consumer, the checkpoint number.
the report:
====================================
BUG: KCSAN: data-race in __add_stripe_bio / raid5d
write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
raid5d+0x1d9/0xba0
[.....]
read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
__add_stripe_bio+0x332/0x400
raid5_make_request+0x6ac/0x2930
md_handle_request+0x4a2/0xa40
md_submit_bio+0x109/0x1a0
__submit_bio+0x2ec/0x390
[.....]
v1 -> v2:
- remove WRITE_ONCE(conf->seq_write) in held device_lock path.
- remove READ_ONCE(conf->seq_flush) in held device_lock path.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a320b71d7117..b2c5a1930841 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3536,11 +3536,11 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
pr_debug("added bi b#%llu to stripe s#%llu, disk %d, logical %llu\n",
(*bip)->bi_iter.bi_sector, sh->sector, dd_idx,
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);
}
}
/*
@@ -5827,11 +5827,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
md_write_inc(mddev, bi);
sh->overwrite_disks++;
}
spin_unlock_irq(&sh->stripe_lock);
if (conf->mddev->bitmap) {
- sh->bm_seq = conf->seq_flush + 1;
+ sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
@@ -6877,16 +6877,17 @@ static void raid5d(struct md_thread *thread)
clear_bit(R5_DID_ALLOC, &conf->cache_state);
if (
!list_empty(&conf->bitmap_list)) {
/* Now is a good time to flush some bitmap updates */
- conf->seq_flush++;
+ int seq = conf->seq_flush + 1;
+ WRITE_ONCE(conf->seq_flush, seq);
spin_unlock_irq(&conf->device_lock);
if (md_bitmap_enabled(mddev, true))
mddev->bitmap_ops->unplug(mddev, true);
spin_lock_irq(&conf->device_lock);
- conf->seq_write = conf->seq_flush;
+ conf->seq_write = seq;
activate_bit_delay(conf, conf->temp_inactive_list);
}
raid5_activate_delayed(conf);
while ((bio = remove_bio_from_retry(conf, &offset))) {
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: sashiko-bot @ 2026-06-17 14:43 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260617142839.882378-1-chencheng@fnnas.com>
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 <chencheng@fnnas.com>
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/20260617142839.882378-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-17 15:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, linux-kernel, linux-crypto, x86, David Laight,
linux-raid
In-Reply-To: <20260617055653.GB19218@lst.de>
On Wed, Jun 17, 2026 at 07:56:53AM +0200, Christoph Hellwig wrote:
> Can use the xor: prefix used for all other commits to lib/raid/xor?
>
> > Benchmark on AMD Ryzen 9 9950X (Zen 5):
> >
> > src_cnt avx avx512 Improvement
> > ======= ========== ========== ===========
> > 1 56353 MB/s 75388 MB/s 33%
> > 2 54274 MB/s 68409 MB/s 26%
> > 3 44649 MB/s 64042 MB/s 43%
> > 4 41315 MB/s 55002 MB/s 33%
>
> On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing
> AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not
> sure if is just the different benchmarking or something else going on.
I used lib/raid/xor/xor-core.c which measures the throughput of parity
data generated, whereas your proposed xor_benchmark() in xor_kunit
measures the throughput of source data consumed. I don't know which
makes more sense, but we should make them consistent with each other.
> FYI, one or 2 sources are basically useless as they RAID5 configs
> that have no benefits over simple mirroring and thus the numbers
> aren't too interesting.
>
> > +DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
> > + xor_avx512_5);
>
> Is there really much of a benefit of doing the historic DO_XOR_BLOCKS
> vs doing the loop manually? Especially as the common cases for a
> modern RAID will usually loop over more disks than this was built
> for. I.e., in practice one or two source buffers only happen at the
> end of a loop over more disks.
There's not really a way out of unrolling by source buffer count, as
otherwise the pointers would continuously have to be reloaded into
registers. That's why your proposal was so slow (see the numbers I gave
in https://lore.kernel.org/linux-crypto/20260612055933.GA6675@sol/ ).
It could be something different from 2-5 specifically, or open-coded
instead of using the macro if that's all you're asking for, but at a
high level the unrolling by source buffer count does seem to be needed.
- Eric
^ permalink raw reply
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Eric Biggers @ 2026-06-17 17:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260617054416.3841334-3-hch@lst.de>
On Wed, Jun 17, 2026 at 07:44:04AM +0200, Christoph Hellwig wrote:
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25..5939b78b3944 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
> @@ -125,8 +125,71 @@ static void xor_test(struct kunit *test)
> }
> }
>
> +#ifdef CONFIG_XOR_BENCHMARK
> +static void xor_benchmark(struct kunit *test)
The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
tests do:
if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
kunit_skip(test, "not enabled");
> + for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> + unsigned int len = len_to_test[j];
> + const unsigned long num_iters = 1000;
> +
> + KUNIT_ASSERT_GT(test, len, 0);
> + KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
> +
> + preempt_disable();
> + t = ktime_get();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = ktime_get_ns() - t;
> + preempt_enable();
First one should be ktime_get_ns(), not ktime_get().
> +
> + speed[j] = div_u64((u64)len * num_iters * nr, t);
> + }
> +
> + static_assert(ARRAY_SIZE(len_to_test) == 2);
> + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> + nr, speed[0], speed[1]);
As mentioned in the other thread, this measures the speed at which the
source data is consumed, which differs from the code in
lib/raid/xor/xor-core.c that measures the speed at which the destination
data is produced. Probably best to make them consistent.
- Eric
^ permalink raw reply
* [PATCH] md/raid10: protect head_position accesses consistency
From: Chen Cheng @ 2026-06-18 2:52 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
the completion paths update conf->mirrors[x].head_position
without a lock, while read_balance() reads the same field
locklessly.
protect conf->mirror[x].head_position consistency for
multi-CPUs by READ_ONCE()/WRITE_ONCE().
the report #1:
=====================
BUG: KCSAN: data-race in find_bio_disk.isra.0 / read_balance
write to 0xffff8f0309679fd0 of 8 bytes by interrupt on cpu 9:
find_bio_disk.isra.0+0x108/0x180
raid10_end_write_request+0xd2/0x530
bio_endio+0x3c9/0x3e0
[...]
read to 0xffff8f0309679fd0 of 8 bytes by task 338766 on cpu 7:
read_balance+0x2ec/0x700
raid10_read_request+0x17b/0x550
raid10_make_request+0x27e/0x1080
md_handle_request+0x2c5/0x700
[...]
value changed: 0x000000000032f440 -> 0x000000000014f8e0
the report #2:
=======================
BUG: KCSAN: data-race in raid10_end_read_request / read_balance
write to 0xffff8f03291091b0 of 8 bytes by interrupt on cpu 9:
raid10_end_read_request+0xf0/0x400
bio_endio+0x3c9/0x3e0
[...]
read to 0xffff8f03291091b0 of 8 bytes by task 336943 on cpu 8:
read_balance+0x2ec/0x700
raid10_read_request+0x17b/0x550
raid10_make_request+0x27e/0x1080
md_handle_request+0x2c5/0x700
[...]
value changed: 0x000000000041c188 -> 0x000000000041c1b8
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid10.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index caf1da15aa87..55aa052a957b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -343,12 +343,12 @@ static void raid_end_bio_io(struct r10bio *r10_bio)
*/
static inline void update_head_pos(int slot, struct r10bio *r10_bio)
{
struct r10conf *conf = r10_bio->mddev->private;
- conf->mirrors[r10_bio->devs[slot].devnum].head_position =
- r10_bio->devs[slot].addr + (r10_bio->sectors);
+ WRITE_ONCE(conf->mirrors[r10_bio->devs[slot].devnum].head_position,
+ r10_bio->devs[slot].addr + (r10_bio->sectors));
}
/*
* Find the disk number which triggered given bio
*/
@@ -841,11 +841,11 @@ static struct md_rdev *read_balance(struct r10conf *conf,
/* for far > 1 always use the lowest address */
else if (geo->far_copies > 1)
new_distance = r10_bio->devs[slot].addr;
else
new_distance = abs(r10_bio->devs[slot].addr -
- conf->mirrors[disk].head_position);
+ READ_ONCE(conf->mirrors[disk].head_position));
if (new_distance < best_dist) {
best_dist = new_distance;
best_dist_slot = slot;
best_dist_rdev = rdev;
--
2.54.0
^ permalink raw reply related
* [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: Chen Cheng @ 2026-06-18 2:57 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
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
^ permalink raw reply related
* Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: sashiko-bot @ 2026-06-18 3:12 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260618025735.915113-1-chencheng@fnnas.com>
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
^ permalink raw reply
* Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: Chen Cheng @ 2026-06-18 3:41 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260618031232.831A41F000E9@smtp.kernel.org>
在 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 .
^ permalink raw reply
* [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 6:55 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
bm_seq means "stripe delay to flush until bm_seq <= seq_write".
do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
when bm_seq >= seq_write.
after raid5d() flushes bitmap update and ++seq_write, and
active_bit_delay() retry to release delayed stripes.
the stripe batch head must carry the newest bm_seq among all
member stripes, because the whole batch later released according
to the batch head state and bm_seq.
race scenario:
===================
1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
3. cpu1 write 102, and cpu0 overwrite with 101;
the point is, if the head has a lower bm_seq than one of its
members, the whole batch could be released before that
member's bitmap is flushed.
and the on-disk bitmap not record sh1's changes.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a08230aac711..ee145a7bf9e8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
/*
* at this point, head's BATCH_READY could be cleared, but we
* can still add the stripe to batch list
*/
list_add(&sh->batch_list, &head->batch_list);
- spin_unlock(&head->batch_head->batch_lock);
} else {
head->batch_head = head;
sh->batch_head = head->batch_head;
spin_lock(&head->batch_lock);
list_add_tail(&sh->batch_list, &head->batch_list);
- spin_unlock(&head->batch_lock);
}
- if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
- if (atomic_dec_return(&conf->preread_active_stripes)
- < IO_THRESHOLD)
- 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 > 0)
seq = sh->batch_head->bm_seq;
set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
sh->batch_head->bm_seq = seq;
}
+ spin_unlock(&head->batch_head->batch_lock);
+
+ if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+ if (atomic_dec_return(&conf->preread_active_stripes)
+ < IO_THRESHOLD)
+ md_wakeup_thread(conf->mddev->thread);
atomic_inc(&sh->count);
unlock_out:
unlock_two_stripes(head, sh);
out:
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Christoph Hellwig @ 2026-06-18 9:22 UTC (permalink / raw)
To: Eric Biggers; +Cc: Christoph Hellwig, Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260617171448.GD785086@google.com>
On Wed, Jun 17, 2026 at 05:14:48PM +0000, Eric Biggers wrote:
> The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
> tests do:
>
> if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
> kunit_skip(test, "not enabled");
I saw that, but what's the point or just not compiling it?
> > + static_assert(ARRAY_SIZE(len_to_test) == 2);
> > + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> > + nr, speed[0], speed[1]);
>
> As mentioned in the other thread, this measures the speed at which the
> source data is consumed, which differs from the code in
> lib/raid/xor/xor-core.c that measures the speed at which the destination
> data is produced. Probably best to make them consistent.
The throughput for the generated parity isn't very interesting. It is
the overall throughput that determines the performance. And yes,
the selection benchmark is a complete mess - the single disk XOR is
not a relevant performance metric.
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Christoph Hellwig @ 2026-06-18 9:25 UTC (permalink / raw)
To: David Laight
Cc: Christoph Hellwig, Eric Biggers, Andrew Morton, linux-kernel,
linux-crypto, x86, linux-raid
In-Reply-To: <20260617110516.0a70950e@pumpkin>
On Wed, Jun 17, 2026 at 11:05:16AM +0100, David Laight wrote:
> > FYI, one or 2 sources are basically useless as they RAID5 configs
> > that have no benefits over simple mirroring and thus the numbers
> > aren't too interesting.
>
> With three disks you xor two buffers (src_count == 1) to get the parity
> to write to the third - so that is a valid RAID5 config.
Sure. It did not say it is invalid, it just isn't very useful.
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-18 9:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Eric Biggers, Andrew Morton, linux-kernel, linux-crypto, x86,
linux-raid
In-Reply-To: <20260618092500.GC17530@lst.de>
On Thu, 18 Jun 2026 11:25:00 +0200
Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jun 17, 2026 at 11:05:16AM +0100, David Laight wrote:
> > > FYI, one or 2 sources are basically useless as they RAID5 configs
> > > that have no benefits over simple mirroring and thus the numbers
> > > aren't too interesting.
> >
> > With three disks you xor two buffers (src_count == 1) to get the parity
> > to write to the third - so that is a valid RAID5 config.
>
> Sure. It did not say it is invalid, it just isn't very useful.
>
It gives you 2/3 the total capacity, mirroring only gives 1/2.
But for a lot of uses I suspect disks are cheap enough that mirroring
is usually the 'best bet'.
It also means that there are a set of catastrophic failures that can
be more easily recovered.
It is probably more important to ensure all the disks in a raid set
come from different production lines - otherwise they are likely to
fail at the same time.
At least modern disks don't (seem to) suffer from 'sticky oil' which
would tend to stop them restarting after they got cold while turned off.
Raid doesn't help when multiple disks fail to start!
David
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Paul Menzel @ 2026-06-18 10:36 UTC (permalink / raw)
To: Chen Cheng; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <20260618065544.954309-1-chencheng@fnnas.com>
Dear Chen,
Thank you for very much.
Am 18.06.26 um 08:55 schrieb Chen Cheng:
> From: Chen Cheng <chencheng@fnnas.com>
>
> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>
> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
> when bm_seq >= seq_write.
>
> after raid5d() flushes bitmap update and ++seq_write, and
> active_bit_delay() retry to release delayed stripes.
>
> the stripe batch head must carry the newest bm_seq among all
> member stripes, because the whole batch later released according
> to the batch head state and bm_seq.
>
> race scenario:
> ===================
> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
> 3. cpu1 write 102, and cpu0 overwrite with 101;
>
> the point is, if the head has a lower bm_seq than one of its
> members, the whole batch could be released before that
> member's bitmap is flushed.
> and the on-disk bitmap not record sh1's changes.
It’s a little hard to read. Could you please improve the wording of the
last paragraph, and maybe also start each sentence with a capital
letter. Maybe also use 75 characters per line.
Do you have a reproducer by any chance?
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
Also add a Fixes: tag?
> ---
> drivers/md/raid5.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a08230aac711..ee145a7bf9e8 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> /*
> * at this point, head's BATCH_READY could be cleared, but we
> * can still add the stripe to batch list
> */
> list_add(&sh->batch_list, &head->batch_list);
> - spin_unlock(&head->batch_head->batch_lock);
> } else {
> head->batch_head = head;
> sh->batch_head = head->batch_head;
> spin_lock(&head->batch_lock);
> list_add_tail(&sh->batch_list, &head->batch_list);
> - spin_unlock(&head->batch_lock);
> }
>
> - if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> - if (atomic_dec_return(&conf->preread_active_stripes)
> - < IO_THRESHOLD)
> - 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 > 0)
> seq = sh->batch_head->bm_seq;
> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
> sh->batch_head->bm_seq = seq;
> }
> + spin_unlock(&head->batch_head->batch_lock);
> +
> + if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> + if (atomic_dec_return(&conf->preread_active_stripes)
> + < IO_THRESHOLD)
> + md_wakeup_thread(conf->mddev->thread);
>
> atomic_inc(&sh->count);
> unlock_out:
> unlock_two_stripes(head, sh);
> out:
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 11:26 UTC (permalink / raw)
To: Paul Menzel; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <5e6ce9e2-4da9-4ded-be8c-fad3ee153d90@molgen.mpg.de>
在 2026/6/18 18:36, Paul Menzel 写道:
> Dear Chen,
>
>
> Thank you for very much.
>
> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>
>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>> when bm_seq >= seq_write.
>>
>> after raid5d() flushes bitmap update and ++seq_write, and
>> active_bit_delay() retry to release delayed stripes.
>>
>> the stripe batch head must carry the newest bm_seq among all
>> member stripes, because the whole batch later released according
>> to the batch head state and bm_seq.
>>
>> race scenario:
>> ===================
>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>
>> the point is, if the head has a lower bm_seq than one of its
>> members, the whole batch could be released before that
>> member's bitmap is flushed.
>> and the on-disk bitmap not record sh1's changes.
>
> It’s a little hard to read. Could you please improve the wording of the
> last paragraph, and maybe also start each sentence with a capital
> letter. Maybe also use 75 characters per line.
>
> Do you have a reproducer by any chance?
Hi Paul,
Thanks to review, and , I will follow your advise.
Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
for this one. Because it's reported by sashiko-review bot, and , I think
it's a true risk.
I will try to make a reproducer for this case later , after I figure-out
the other KCSAN reports.
>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>
> Also add a Fixes: tag?
>
>> ---
>> drivers/md/raid5.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index a08230aac711..ee145a7bf9e8 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct
>> r5conf *conf,
>> /*
>> * at this point, head's BATCH_READY could be cleared, but we
>> * can still add the stripe to batch list
>> */
>> list_add(&sh->batch_list, &head->batch_list);
>> - spin_unlock(&head->batch_head->batch_lock);
>> } else {
>> head->batch_head = head;
>> sh->batch_head = head->batch_head;
>> spin_lock(&head->batch_lock);
>> list_add_tail(&sh->batch_list, &head->batch_list);
>> - spin_unlock(&head->batch_lock);
>> }
>> - if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>> - if (atomic_dec_return(&conf->preread_active_stripes)
>> - < IO_THRESHOLD)
>> - 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 > 0)
>> seq = sh->batch_head->bm_seq;
>> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
>> sh->batch_head->bm_seq = seq;
>> }
>> + spin_unlock(&head->batch_head->batch_lock);
>> +
>> + if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>> + if (atomic_dec_return(&conf->preread_active_stripes)
>> + < IO_THRESHOLD)
>> + md_wakeup_thread(conf->mddev->thread);
>> atomic_inc(&sh->count);
>> unlock_out:
>> unlock_two_stripes(head, sh);
>> out:
>
>
> Kind regards,
>
> Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Paul Menzel @ 2026-06-18 12:15 UTC (permalink / raw)
To: Chen Cheng; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <76dd7a7b-b1ad-4f61-9eda-5957f712ed87@fnnas.com>
Dear Chen,
Am 18.06.26 um 13:26 schrieb Chen Cheng:
> 在 2026/6/18 18:36, Paul Menzel 写道:
>> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>>> From: Chen Cheng <chencheng@fnnas.com>
>>>
>>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>>
>>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>>> when bm_seq >= seq_write.
>>>
>>> after raid5d() flushes bitmap update and ++seq_write, and
>>> active_bit_delay() retry to release delayed stripes.
>>>
>>> the stripe batch head must carry the newest bm_seq among all
>>> member stripes, because the whole batch later released according
>>> to the batch head state and bm_seq.
>>>
>>> race scenario:
>>> ===================
>>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>>
>>> the point is, if the head has a lower bm_seq than one of its
>>> members, the whole batch could be released before that
>>> member's bitmap is flushed.
>>> and the on-disk bitmap not record sh1's changes.
>>
>> It’s a little hard to read. Could you please improve the wording of the
>> last paragraph, and maybe also start each sentence with a capital
>> letter. Maybe also use 75 characters per line.
>>
>> Do you have a reproducer by any chance?
> Thanks to review, and , I will follow your advise.
Thank you for reading my comments.
> Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
> for this one. Because it's reported by sashiko-review bot, and, I think
> it's a true risk.
Maybe also mention sashiko-review.
> I will try to make a reproducer for this case later, after I figure-out
> the other KCSAN reports.
A reproducer is not required, I was just interested, how the issue was
found. So don’t spend too much on it or at all.
[…]
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 12:30 UTC (permalink / raw)
To: Paul Menzel, Chen Cheng; +Cc: linux-raid, yukuai, yukuai, linux-kernel
In-Reply-To: <946e31d4-1ea8-4c65-be0a-0f942a9cdf8c@molgen.mpg.de>
在 2026/6/18 20:15, Paul Menzel 写道:
> Dear Chen,
>
>
> Am 18.06.26 um 13:26 schrieb Chen Cheng:
>> 在 2026/6/18 18:36, Paul Menzel 写道:
>
>>> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>>>> From: Chen Cheng <chencheng@fnnas.com>
>>>>
>>>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>>>
>>>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>>>> when bm_seq >= seq_write.
>>>>
>>>> after raid5d() flushes bitmap update and ++seq_write, and
>>>> active_bit_delay() retry to release delayed stripes.
>>>>
>>>> the stripe batch head must carry the newest bm_seq among all
>>>> member stripes, because the whole batch later released according
>>>> to the batch head state and bm_seq.
>>>>
>>>> race scenario:
>>>> ===================
>>>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>>>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>>>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>>>
>>>> the point is, if the head has a lower bm_seq than one of its
>>>> members, the whole batch could be released before that
>>>> member's bitmap is flushed.
>>>> and the on-disk bitmap not record sh1's changes.
>>>
>>> It’s a little hard to read. Could you please improve the wording of the
>>> last paragraph, and maybe also start each sentence with a capital
>>> letter. Maybe also use 75 characters per line.
>>>
>>> Do you have a reproducer by any chance?
>
>> Thanks to review, and , I will follow your advise.
>
> Thank you for reading my comments.
>
>> Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
>> for this one. Because it's reported by sashiko-review bot, and, I think
>> it's a true risk.
>
> Maybe also mention sashiko-review.
>
>> I will try to make a reproducer for this case later, after I figure-out
>> the other KCSAN reports.
>
> A reproducer is not required, I was just interested, how the issue was
> found. So don’t spend too much on it or at all.
Well , I think a good reproducer has to be :
- Concurrency as much path as possible ,
e.g. reshape concurrency with normal I/O, etc...
Optionally ,
- Run some Sanitizer,
- Inject some fault, e.g. bad block, write error, disk drop...
- Provide some background I/O pressure,
- Do some combinations describes above
>
> […]
>
>
> Kind regards,
>
> Paul
^ permalink raw reply
* [PATCH] md/raid5: use stripe state snapshot in break_stripe_batch_list()
From: Chen Cheng @ 2026-06-18 13:47 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
The patch just suppress KCSAN noise. No functional change.
RAID-5 can group multi full-stripe-write aka stripe_head into a
batch aka batch_list, with one head_sh leading them. Call
break_stripe_batch_list() when the batch is finished, or,
a stripe has to be dropped out of the batch.
break_stripe_batch_list() reads stripe state several times while
request paths can update thost state words concurrently with
lockless bitops, which reported by KCSAN.
Use a snapshot to guarantees that the value used for
warning, copying, and handle checks is internally consistent
at current read moment.
KCSAN report:
==============================================
BUG: KCSAN: data-race in __add_stripe_bio / break_stripe_batch_list
write (marked) to 0xffff8e89d4f0b988 of 8 bytes by task 4323 on cpu 3:
__add_stripe_bio+0x35e/0x400
raid5_make_request+0x6ac/0x2930
md_handle_request+0x4a2/0xa40
md_submit_bio+0x109/0x1a0
__submit_bio+0x2ec/0x390
submit_bio_noacct_nocheck+0x457/0x710
submit_bio_noacct+0x2a7/0xc20
submit_bio+0x56/0x250
blkdev_direct_IO+0x54c/0xda0
blkdev_write_iter+0x38f/0x570
aio_write+0x22b/0x490
io_submit_one+0xa51/0xf70
read to 0xffff8e89d4f0b988 of 8 bytes by task 4290 on cpu 4:
break_stripe_batch_list+0x3ce/0x480
handle_stripe_clean_event+0x720/0x9b0
handle_stripe+0x32fb/0x4500
handle_active_stripes.isra.0+0x6e0/0xa50
raid5d+0x7e0/0xba0
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 26c24986e01c..a376560be92e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -4906,35 +4906,39 @@ static int clear_batch_ready(struct stripe_head *sh)
static void break_stripe_batch_list(struct stripe_head *head_sh,
unsigned long handle_flags)
{
struct stripe_head *sh, *next;
int i;
+ unsigned long state;
list_for_each_entry_safe(sh, next, &head_sh->batch_list, batch_list) {
list_del_init(&sh->batch_list);
- WARN_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
- (1 << STRIPE_SYNCING) |
- (1 << STRIPE_REPLACED) |
- (1 << STRIPE_DELAYED) |
- (1 << STRIPE_BIT_DELAY) |
- (1 << STRIPE_FULL_WRITE) |
- (1 << STRIPE_BIOFILL_RUN) |
- (1 << STRIPE_COMPUTE_RUN) |
- (1 << STRIPE_DISCARD) |
- (1 << STRIPE_BATCH_READY) |
- (1 << STRIPE_BATCH_ERR)),
- "stripe state: %lx\n", sh->state);
- WARN_ONCE(head_sh->state & ((1 << STRIPE_DISCARD) |
- (1 << STRIPE_REPLACED)),
- "head stripe state: %lx\n", head_sh->state);
+ state = READ_ONCE(sh->state);
+ WARN_ONCE(state & ((1 << STRIPE_ACTIVE) |
+ (1 << STRIPE_SYNCING) |
+ (1 << STRIPE_REPLACED) |
+ (1 << STRIPE_DELAYED) |
+ (1 << STRIPE_BIT_DELAY) |
+ (1 << STRIPE_FULL_WRITE) |
+ (1 << STRIPE_BIOFILL_RUN) |
+ (1 << STRIPE_COMPUTE_RUN) |
+ (1 << STRIPE_DISCARD) |
+ (1 << STRIPE_BATCH_READY) |
+ (1 << STRIPE_BATCH_ERR)),
+ "stripe state: %lx\n", state);
+
+ state = READ_ONCE(head_sh->state);
+ WARN_ONCE(state & ((1 << STRIPE_DISCARD) |
+ (1 << STRIPE_REPLACED)),
+ "head stripe state: %lx\n", state);
set_mask_bits(&sh->state, ~(STRIPE_EXPAND_SYNC_FLAGS |
(1 << STRIPE_PREREAD_ACTIVE) |
(1 << STRIPE_ON_UNPLUG_LIST)),
- head_sh->state & (1 << STRIPE_INSYNC));
+ state & (1 << STRIPE_INSYNC));
sh->check_state = head_sh->check_state;
sh->reconstruct_state = head_sh->reconstruct_state;
spin_lock_irq(&sh->stripe_lock);
sh->batch_head = NULL;
@@ -4943,22 +4947,25 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
wake_up_bit(&sh->dev[i].flags, R5_Overlap);
sh->dev[i].flags = head_sh->dev[i].flags &
(~((1 << R5_WriteError) | (1 << R5_Overlap)));
}
- if (handle_flags == 0 ||
- sh->state & handle_flags)
+
+ state = READ_ONCE(sh->state);
+ if (handle_flags == 0 || (state & handle_flags))
set_bit(STRIPE_HANDLE, &sh->state);
raid5_release_stripe(sh);
}
spin_lock_irq(&head_sh->stripe_lock);
head_sh->batch_head = NULL;
spin_unlock_irq(&head_sh->stripe_lock);
for (i = 0; i < head_sh->disks; i++)
if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
wake_up_bit(&head_sh->dev[i].flags, R5_Overlap);
- if (head_sh->state & handle_flags)
+
+ state = READ_ONCE(head_sh->state);
+ if (state & handle_flags)
set_bit(STRIPE_HANDLE, &head_sh->state);
}
/*
* handle_stripe - do things to a stripe.
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Eric Biggers @ 2026-06-18 17:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260618092224.GB17530@lst.de>
On Thu, Jun 18, 2026 at 11:22:24AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 17, 2026 at 05:14:48PM +0000, Eric Biggers wrote:
> > The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
> > tests do:
> >
> > if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
> > kunit_skip(test, "not enabled");
>
> I saw that, but what's the point or just not compiling it?
Just the usual point of avoiding #ifdefs. Developers won't have to
build the file with two different kconfigs to ensure it builds.
- Eric
^ permalink raw reply
* [PATCH] md/raid5: avoid R5_Overlap races while breaking stripe batches
From: Chen Cheng @ 2026-06-19 4:10 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
KCSAN report a race in break_stripe_batch_list() vs. raid5_make_request()
on sh->dev[i].flags (plain word write vs. atomic bit op)..
and .. one possible scenario is:
CPU1 CPU2
break_stripe_batch_list(sh1)
-> handle sh2
-> lock(sh2)
-> sh2->batch_head = NULL
-> unlock(sh2)
-> test_and_clear_bit(R5_Overlap, sh2->dev[i].flags)
-> wake_up_bit(sh2->dev[i].flags)
raid5_make_request()
-> add_all_stripe_bios(sh2)
-> lock(sh2)
-> stripe_bio_overlaps(sh2) returns true
batch_head is NULL, so new bio overlap
exist bio on sh2 -> true
-> set_bit(R5_Overlap, sh2->dev[i].flags)
-> unlock(sh2)
-> wait_on_bit(sh2->dev[i].flags)
-> sh2->dev[i].flags = sh1->dev[i].flags & ~R5_Overlap
No wait_up_bit(), CPU2 could be wait_on_bit() forever...
Fix by :
- Expand the protect zone.
- Use batch_head's device flag's snaphot when no held head_sh->stripe_lock.
- Move sh/head_sh->batch_head = NULL to the end of protected zone , and ,
any concurrent add_all_stripe_bios() grabs sh->stripe_lock now either:
- see batch_head != null, and , is rejected by stripe_bio_overlaps()
under the lock (no R5_Overlap wait ) , or ,
- sees batch_head == NULL, only after dev[i].flags has already been
set and the prior R5_Overlap waiters worken.
KCSAN report:
================================================
BUG: KCSAN: data-race in break_stripe_batch_list / raid5_make_request
write (marked) to 0xffff8e89c8117548 of 8 bytes by task 4042 on cpu 0:
raid5_make_request+0xea0/0x2930
md_handle_request+0x4a2/0xa40
md_submit_bio+0x109/0x1a0
__submit_bio+0x2ec/0x390
submit_bio_noacct_nocheck+0x457/0x710
submit_bio_noacct+0x2a7/0xc20
submit_bio+0x56/0x250
blkdev_direct_IO+0x54c/0xda0
blkdev_write_iter+0x38f/0x570
aio_write+0x22b/0x490
io_submit_one+0xa51/0xf70
__x64_sys_io_submit+0xf7/0x220
x64_sys_call+0x1907/0x1c60
do_syscall_64+0x130/0x570
entry_SYSCALL_64_after_hwframe+0x76/0x7e
read to 0xffff8e89c8117548 of 8 bytes by task 4010 on cpu 5:
break_stripe_batch_list+0x249/0x480
handle_stripe_clean_event+0x720/0x9b0
handle_stripe+0x32fb/0x4500
handle_active_stripes.isra.0+0x6e0/0xa50
raid5d+0x7e0/0xba0
md_thread+0x15a/0x2d0
kthread+0x1e3/0x220
ret_from_fork+0x37a/0x410
ret_from_fork_asm+0x1a/0x30
value changed: 0x0000000000000019 -> 0x0000000000000099 --> R5_Overlap
Fixes: fb642b92c267b
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a376560be92e..5521051a9425 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -4939,30 +4939,30 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
state & (1 << STRIPE_INSYNC));
sh->check_state = head_sh->check_state;
sh->reconstruct_state = head_sh->reconstruct_state;
spin_lock_irq(&sh->stripe_lock);
- sh->batch_head = NULL;
- spin_unlock_irq(&sh->stripe_lock);
for (i = 0; i < sh->disks; i++) {
if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
wake_up_bit(&sh->dev[i].flags, R5_Overlap);
- sh->dev[i].flags = head_sh->dev[i].flags &
+ sh->dev[i].flags = READ_ONCE(head_sh->dev[i].flags) &
(~((1 << R5_WriteError) | (1 << R5_Overlap)));
}
+ sh->batch_head = NULL;
+ spin_unlock_irq(&sh->stripe_lock);
state = READ_ONCE(sh->state);
if (handle_flags == 0 || (state & handle_flags))
set_bit(STRIPE_HANDLE, &sh->state);
raid5_release_stripe(sh);
}
spin_lock_irq(&head_sh->stripe_lock);
- head_sh->batch_head = NULL;
- spin_unlock_irq(&head_sh->stripe_lock);
for (i = 0; i < head_sh->disks; i++)
if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
wake_up_bit(&head_sh->dev[i].flags, R5_Overlap);
+ head_sh->batch_head = NULL;
+ spin_unlock_irq(&head_sh->stripe_lock);
state = READ_ONCE(head_sh->state);
if (state & handle_flags)
set_bit(STRIPE_HANDLE, &head_sh->state);
}
--
2.54.0
^ permalink raw reply related
* [PATCH] md/raid1: protect head_position for read balance
From: Chen Cheng @ 2026-06-19 4:41 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
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().
KCSAN report:
=========================
BUG: KCSAN: data-race in raid1_end_read_request / raid1_read_request
write to 0xffff8f0306ba7868 of 8 bytes by interrupt on cpu 9:
raid1_end_read_request+0xb5/0x440
bio_endio+0x3c9/0x3e0
blk_update_request+0x257/0x770
scsi_end_request+0x4d/0x520
scsi_io_completion+0x6f/0x990
scsi_finish_command+0x188/0x280
scsi_complete+0xac/0x160
blk_complete_reqs+0x8e/0xb0
blk_done_softirq+0x1d/0x30
[...]
read to 0xffff8f0306ba7868 of 8 bytes by task 667002 on cpu 11:
raid1_read_request+0x497/0x1a10
raid1_make_request+0xdf/0x1950
md_handle_request+0x2c5/0x700
md_submit_bio+0x126/0x320
__submit_bio+0x2ec/0x3a0
submit_bio_noacct_nocheck+0x572/0x890
[...]
value changed: 0x0000000000000078 -> 0x00000000005fe448
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid1.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 5b9368bd9e70..4cdf4484cab6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -357,12 +357,12 @@ static void raid_end_bio_io(struct r1bio *r1_bio)
*/
static inline void update_head_pos(int disk, struct r1bio *r1_bio)
{
struct r1conf *conf = r1_bio->mddev->private;
- conf->mirrors[disk].head_position =
- r1_bio->sector + (r1_bio->sectors);
+ WRITE_ONCE(conf->mirrors[disk].head_position,
+ r1_bio->sector + r1_bio->sectors);
}
/*
* Find the disk number which triggered given bio
*/
@@ -735,11 +735,11 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
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;
}
/*
* If buffered sequential IO size exceeds optimal iosize, check if there is idle
* disk. If yes, choose the idle disk.
@@ -812,11 +812,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
/* At least two disks to choose from so failfast is OK */
if (ctl.readable_disks++ == 1)
set_bit(R1BIO_FailFast, &r1_bio->state);
pending = atomic_read(&rdev->nr_pending);
- dist = abs(r1_bio->sector - conf->mirrors[disk].head_position);
+ dist = abs(r1_bio->sector -
+ READ_ONCE(conf->mirrors[disk].head_position));
/* Don't change to another disk for sequential reads */
if (is_sequential(conf, disk, r1_bio)) {
if (!should_choose_next(conf, disk))
return disk;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/raid1: protect head_position for read balance
From: sashiko-bot @ 2026-06-19 4:51 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260619044114.1208456-1-chencheng@fnnas.com>
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?
[ ... ]
> 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?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619044114.1208456-1-chencheng@fnnas.com?part=1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox