* [PATCH 0/7] md/raid10: fixes, atomic write handling, and error-path cleanup
From: Abd-Alrhman Masalkhi @ 2026-06-23 7:24 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, john.g.garry,
martin.petersen, abd.masalkhi
Cc: linux-raid, linux-kernel
Hi,
This series contains a mix of bug fixes and cleanups for RAID10, along
with a related atomic write fix for RAID1.
Thanks,
Abd-alrhman,
Abd-Alrhman Masalkhi (7):
md/raid10: fix r10bio leak in raid10_write_request() error paths
md/raid1: handle atomic writes that require splitting
md/raid10: handle atomic writes that require splitting
md/raid10: raid10_write_request() drops the barrier before calling
md/raid10: replace wait loop with wait_event_idle()
md/raid10: simplify write request error handling
md/raid10: simplify read request error handling
drivers/md/raid1.c | 25 ++++-----
drivers/md/raid10.c | 123 +++++++++++++++++++++-----------------------
2 files changed, 71 insertions(+), 77 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v4 1/3] md: suspend array before raid10 reshape via sync_action
From: Chen Cheng @ 2026-06-23 4:29 UTC (permalink / raw)
To: yukuai, linux-raid; +Cc: linux-kernel
In-Reply-To: <9919b1ec-66ca-4f91-af58-fdc34c0f8273@fygo.io>
在 2026/6/21 03:43, yu kuai 写道:
> Hi,
>
> 在 2026/6/3 11:59, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> The sync_action=reshape path currently enters mddev_start_reshape() with
>> reconfig_mutex held but without suspending the array first. For raid10,
>> that means raid10_start_reshape() has to drop reconfig_mutex and reacquire
>> the array through mddev_suspend_and_lock_nointr() before it can safely
>> switch geometry-dependent state.
>>
>> Use mddev_suspend_and_lock() for ACTION_RESHAPE in action_store(), so
>> the sysfs reshape path reaches mddev_start_reshape() with the array
>> already suspended and locked.
>>
>> Other sync_action operations keep using mddev_lock() unchanged.
>>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>> ---
>> drivers/md/md.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 096bb64e87bd..5bc937e149ac 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5256,30 +5256,39 @@ static int mddev_start_reshape(struct mddev *mddev)
>>
>> static ssize_t
>> action_store(struct mddev *mddev, const char *page, size_t len)
>> {
>> int ret;
>> + bool suspended = false;
>> enum sync_action action;
>>
>> if (!mddev->pers || !mddev->pers->sync_request)
>> return -EINVAL;
>>
>> + action = md_sync_action_by_name(page);
>> retry:
>> if (work_busy(&mddev->sync_work))
>> flush_work(&mddev->sync_work);
>>
>> - ret = mddev_lock(mddev);
>> + if (action == ACTION_RESHAPE) {
>> + ret = mddev_suspend_and_lock(mddev);
>
> suspend during retry is too heavy. Please move suspend above retry.
hi,
If we move suspend before flush_work , that would have a deadlock window
from sashiko-bot review comment.
code like:
action_store(struct mddev *mddev, const char *page, size_t len)
{
...
+ action = md_sync_action_by_name(page);
+ if (action == ACTION_RESHAPE) {
+ ret = mddev_suspend(mddev, true);
+ if (ret)
+ return ret;
+ suspended = true;
+ }
retry:
if (work_busy(&mddev->sync_work))
flush_work(&mddev->sync_work);
ret = mddev_lock(mddev);
and the scenario is :
reshape:
action_store() -> mddev_suspend() -> flush_work()
mddev_suspend(), mark implicit GFP_NOIO allocation scope for thread#1 by
memalloc_noio_save().
flush_work() call sync_work handler md_start_sync() and would call
kzalloc_obj by flag GFP_KERNEL.
So.. when memory reclaim issue I/O to array and it's suspended,
md_handle_request() wait in !is_suspended(mddev, bio) by sleep , cause
deadlock.
Seems both way is unacceptable ?
>
>> + suspended = true;
>> + } else {
>> + ret = mddev_lock(mddev);
>> + suspended = false;
>> + }
>> if (ret)
>> return ret;
>>
>> if (work_busy(&mddev->sync_work)) {
>> - mddev_unlock(mddev);
>> + if (suspended)
>> + mddev_unlock_and_resume(mddev);
>> + else
>> + mddev_unlock(mddev);
>> goto retry;
>> }
>>
>> - action = md_sync_action_by_name(page);
>> -
>> /* TODO: mdadm rely on "idle" to start sync_thread. */
>> if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
>> switch (action) {
>> case ACTION_FROZEN:
>> md_frozen_sync_thread(mddev);
>> @@ -5344,11 +5353,14 @@ action_store(struct mddev *mddev, const char *page, size_t len)
>> md_wakeup_thread(mddev->thread);
>> sysfs_notify_dirent_safe(mddev->sysfs_action);
>> ret = len;
>>
>> out:
>> - mddev_unlock(mddev);
>> + if (suspended)
>> + mddev_unlock_and_resume(mddev);
>> + else
>> + mddev_unlock(mddev);
>> return ret;
>> }
>>
>> static struct md_sysfs_entry md_scan_mode =
>> __ATTR_PREALLOC(sync_action, S_IRUGO|S_IWUSR, action_show, action_store);
>
^ permalink raw reply
* Re: [PATCH] raid6: fix riscv symbol undeclared warnigns
From: Nam Cao @ 2026-06-22 18:23 UTC (permalink / raw)
To: Ben Dooks, linux-raid, Song Liu, Yu Kuai
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Ben Dooks
In-Reply-To: <20260622135207.480540-1-ben.dooks@codethink.co.uk>
Ben Dooks <ben.dooks@codethink.co.uk> writes:
> The riscv rvv.c file is missing the include of pq_arch.h
> which defines all the exported functions. Include this
> to remove the following sparse warnings:
>
> lib/raid/raid6/riscv/rvv.c:1225:1: warning: symbol 'raid6_rvvx1' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1226:1: warning: symbol 'raid6_rvvx2' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1227:1: warning: symbol 'raid6_rvvx4' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1228:1: warning: symbol 'raid6_rvvx8' was not declared. Should it be static?
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Nam Cao <namcao@linutronix.de>
^ permalink raw reply
* Re: [PATCH] raid6: fix raid6_recov_rvv symbol undeclared warning
From: Nam Cao @ 2026-06-22 18:21 UTC (permalink / raw)
To: Ben Dooks, linux-raid, Song Liu, Yu Kuai
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Ben Dooks
In-Reply-To: <20260622135535.481534-1-ben.dooks@codethink.co.uk>
Ben Dooks <ben.dooks@codethink.co.uk> writes:
> The riscv recov_rvv.c should have included pq_arch.h for
> the definition of raid6_recov_rvv. Add the include to
> fix the following sparse warning:
>
> lib/raid/raid6/riscv/recov_rvv.c:218:32: warning: symbol 'raid6_recov_rvv' was not declared. Should it be static?
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Reviewed-by: Nam Cao <namcao@linutronix.de>
^ permalink raw reply
* [PATCH v2] md/raid5-ppl: fix use-after-free in ppl_do_flush()
From: Sajal Gupta @ 2026-06-22 14:06 UTC (permalink / raw)
To: linux-raid, song
Cc: yukuai3, tomasz.majchrzak, linux-kernel, error27, skhan, me,
linux-kernel-mentees, Sajal Gupta
The loop in ppl_do_flush() continues iterating after calling
ppl_io_unit_finished(), touching io->pending_flushes and leading to a
use-after-free.
Add a break statement to stop the loop once io is freed.
Fixes: 1532d9e87e8b ("raid5-ppl: PPL support for disks with write-back cache enabled")
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/all/ajJF2wKYWRk4GGCK@stanley.mountain/
Signed-off-by: Sajal Gupta <sajal2005gupta@gmail.com>
---
Compile tested only.
Changes in v2:
- drop the refcount_t conversion
v1: https://lore.kernel.org/all/20260622080656.22786-1-sajal2005gupta@gmail.com/
drivers/md/raid5-ppl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
index a70cbec12ed0..c3cfdd66d8b0 100644
--- a/drivers/md/raid5-ppl.c
+++ b/drivers/md/raid5-ppl.c
@@ -643,8 +643,10 @@ static void ppl_do_flush(struct ppl_io_unit *io)
log->disk_flush_bitmap = 0;
for (i = flushed_disks ; i < raid_disks; i++) {
- if (atomic_dec_and_test(&io->pending_flushes))
+ if (atomic_dec_and_test(&io->pending_flushes)) {
ppl_io_unit_finished(io);
+ break;
+ }
}
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] raid6: fix riscv symbol undeclared warnigns
From: sashiko-bot @ 2026-06-22 14:02 UTC (permalink / raw)
To: Ben Dooks; +Cc: yukuai, linux-raid
In-Reply-To: <20260622135207.480540-1-ben.dooks@codethink.co.uk>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Including `pq_arch.h` in `rvv.c` misuses an initialization fragment as a header, creating a brittle include dependency and violating architectural boundaries.
--
commit 0d7480a95df3baf577ec13b2b5aec7d36d11ee3f
Author: Ben Dooks <ben.dooks@codethink.co.uk>
raid6: fix riscv symbol undeclared warnigns
This commit attempts to resolve sparse warnings about undeclared raid6_rvvx
symbols in the RISC-V RAID6 implementation by including pq_arch.h in rvv.c.
> raid6: fix riscv symbol undeclared warnigns
This isn't a bug, but there's a typo (warnigns) in the subject line.
> diff --git a/lib/raid/raid6/riscv/rvv.c b/lib/raid/raid6/riscv/rvv.c
> index 75c9dafedb284..4ac50606f3dc1 100644
> --- a/lib/raid/raid6/riscv/rvv.c
> +++ b/lib/raid/raid6/riscv/rvv.c
> @@ -10,6 +10,7 @@
> */
>
> #include "rvv.h"
> +#include "pq_arch.h"
[Severity: Low]
Is it appropriate to include pq_arch.h here to fix the sparse warnings?
The pq_arch.h file appears to be designed as an initialization fragment meant
to be conditionally included by the generic algos.c, since it contains the
arch_raid6_init() static inline function. Including it here inappropriately
injects unused initialization functions into the backend implementation.
Does this also create a brittle include order dependency? The pq_arch.h file
uses functions like raid6_algo_add_default and raid6_algo_add but does not
include algos.h where they are declared. The build only succeeds because rvv.c
happens to include rvv.h (which includes algos.h) immediately before pq_arch.h.
Any reordering of these includes could break the build.
Would it be safer to place the extern declarations for these symbols in a
shared internal header like rvv.h?
>
> #ifdef __riscv_vector
> #error "This code must be built without compiler support for vector"
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260622135207.480540-1-ben.dooks@codethink.co.uk?part=1
^ permalink raw reply
* [PATCH] raid6: fix raid6_recov_rvv symbol undeclared warning
From: Ben Dooks @ 2026-06-22 13:55 UTC (permalink / raw)
To: linux-raid, Song Liu, Yu Kuai
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Ben Dooks
The riscv recov_rvv.c should have included pq_arch.h for
the definition of raid6_recov_rvv. Add the include to
fix the following sparse warning:
lib/raid/raid6/riscv/recov_rvv.c:218:32: warning: symbol 'raid6_recov_rvv' was not declared. Should it be static?
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
lib/raid/raid6/riscv/recov_rvv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/raid/raid6/riscv/recov_rvv.c b/lib/raid/raid6/riscv/recov_rvv.c
index 2305940276dd..78e158a3e332 100644
--- a/lib/raid/raid6/riscv/recov_rvv.c
+++ b/lib/raid/raid6/riscv/recov_rvv.c
@@ -8,6 +8,7 @@
#include <linux/raid/pq.h>
#include "algos.h"
#include "rvv.h"
+#include "pq_arch.h"
static void __raid6_2data_recov_rvv(int bytes, u8 *p, u8 *q, u8 *dp,
u8 *dq, const u8 *pbmul,
--
2.37.2.352.g3c44437643
^ permalink raw reply related
* [PATCH] raid6: fix riscv symbol undeclared warnigns
From: Ben Dooks @ 2026-06-22 13:52 UTC (permalink / raw)
To: linux-raid, Song Liu, Yu Kuai
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Ben Dooks
The riscv rvv.c file is missing the include of pq_arch.h
which defines all the exported functions. Include this
to remove the following sparse warnings:
lib/raid/raid6/riscv/rvv.c:1225:1: warning: symbol 'raid6_rvvx1' was not declared. Should it be static?
lib/raid/raid6/riscv/rvv.c:1226:1: warning: symbol 'raid6_rvvx2' was not declared. Should it be static?
lib/raid/raid6/riscv/rvv.c:1227:1: warning: symbol 'raid6_rvvx4' was not declared. Should it be static?
lib/raid/raid6/riscv/rvv.c:1228:1: warning: symbol 'raid6_rvvx8' was not declared. Should it be static?
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
lib/raid/raid6/riscv/rvv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/raid/raid6/riscv/rvv.c b/lib/raid/raid6/riscv/rvv.c
index 75c9dafedb28..4ac50606f3dc 100644
--- a/lib/raid/raid6/riscv/rvv.c
+++ b/lib/raid/raid6/riscv/rvv.c
@@ -10,6 +10,7 @@
*/
#include "rvv.h"
+#include "pq_arch.h"
#ifdef __riscv_vector
#error "This code must be built without compiler support for vector"
--
2.37.2.352.g3c44437643
^ permalink raw reply related
* [PATCH] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: Chen Cheng @ 2026-06-22 12:46 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, 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
[.....]
Fixes: 7c13edc87510f ("md: incorporate new plugging into raid5.")
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 | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a320b71d7117..de62ce4c3a21 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,18 @@ 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 v5 2/3] md/raid10: resize r10bio_pool for reshape
From: sashiko-bot @ 2026-06-22 12:42 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260622121312.1775322-3-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing `conf->r10bio_pool` during `raid10_start_reshape()` races with discard I/O completion, leading to mempool Use-After-Free and heap out-of-bounds writes.
--
commit 1d797159800c9f37dc1886dd00e8c5968da7d731
Author: Chen Cheng <chencheng@fnnas.com>
Subject: md/raid10: resize r10bio_pool for reshape
This commit updates raid10_start_reshape() to allocate a new, larger
r10bio_pool when the number of raid disks changes. The old pool is then
destroyed and replaced with the new one.
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -4511,6 +4527,10 @@ static int raid10_start_reshape(struct mddev *mddev)
> mddev->raid_disks = conf->geo.raid_disks;
> mddev->reshape_position = conf->reshape_progress;
> set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> + if (new_pool) {
> + mempool_destroy(conf->r10bio_pool);
> + conf->r10bio_pool = new_pool;
> + }
[Severity: Critical]
Does this introduce a race condition with discard I/O completion?
In the discard completion path, raid_end_discard_bio() calls bio_endio()
on the master bio before freeing the r10bio:
raid_end_discard_bio() {
...
bio_endio(r10bio->master_bio);
...
free_r10bio(r10bio);
}
Calling bio_endio() invokes md_end_clone_io(), which decrements the
mddev->active_io refcount. If this drops to zero, mddev_suspend() can return
while the thread is still executing raid_end_discard_bio().
If that happens, raid10_start_reshape() can proceed to destroy
conf->r10bio_pool and assign new_pool here, while free_r10bio() is still
about to use it.
Could this lead to free_r10bio() releasing the object into a destroyed
mempool, or freeing a smaller r10_bio object into the newly allocated pool
designed for larger objects (which could cause a heap out-of-bounds write
later when __make_request() clears the devs array)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260622121312.1775322-1-chencheng@fnnas.com?part=2
^ permalink raw reply
* Re: [PATCH v5 3/3] md/raid10: free r10bio before ending master_bio in raid_end_bio_io()
From: sashiko-bot @ 2026-06-22 12:29 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260622121312.1775322-4-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix: identical Use-After-Free and mempool corruption in `raid_end_discard_bio`.
- [High] Memory leak of `r10_bio` on `REQ_NOWAIT` write requests during cluster resync or reshape.
--
commit 088539a2820315440079f31df4aa792c8a6bc547
Author: Chen Cheng <chencheng@fnnas.com>
md/raid10: free r10bio before ending master_bio in raid_end_bio_io()
This commit reorders the teardown sequence in raid_end_bio_io() to free
r10_bio before calling bio_endio(). This prevents a race condition where
a concurrent array reshape can alter the geometry and destroy the mempool
after the master bio completes but before the r10_bio is freed.
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index d740744a97461..a4642c903b202 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -332,12 +332,12 @@ static void raid_end_bio_io(struct r10bio *r10_bio)
> {
> struct bio *bio = r10_bio->master_bio;
> struct r10conf *conf = r10_bio->mddev->private;
> + bool returned = test_and_set_bit(R10BIO_Returned, &r10_bio->state);
> + blk_status_t status = test_bit(R10BIO_Uptodate, &r10_bio->state)
> + ? BLK_STS_OK : BLK_STS_IOERR;
>
> - if (!test_and_set_bit(R10BIO_Returned, &r10_bio->state)) {
> - if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
> - bio->bi_status = BLK_STS_IOERR;
> - bio_endio(bio);
> - }
> + put_all_bios(conf, r10_bio);
> + mempool_free(r10_bio, conf->r10bio_pool);
>
> /*
> * Wake up any possible resync thread that waits for the device
> @@ -345,7 +345,10 @@ static void raid_end_bio_io(struct r10bio *r10_bio)
> */
> allow_barrier(conf);
>
> - free_r10bio(r10_bio);
> + if (!returned) {
> + bio->bi_status = status;
> + bio_endio(bio);
> + }
> }
>
[Severity: High]
This is a pre-existing issue, but the exact same race condition appears to
exist in the discard completion path.
In drivers/md/raid10.c:raid_end_discard_bio():
} else {
md_write_end(r10bio->mddev);
bio_endio(r10bio->master_bio);
free_r10bio(r10bio);
break;
}
Since bio_endio(r10bio->master_bio) drops active_io to zero, couldn't this
also wake up mddev_suspend() and allow a concurrent reshape to modify
conf->geo and destroy conf->r10bio_pool before free_r10bio() is called?
[Severity: High]
This is a pre-existing issue, but does raid10_write_request() leak the r10_bio
on REQ_NOWAIT requests?
In drivers/md/raid10.c:__make_request():
r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
...
if (bio_data_dir(bio) == READ)
raid10_read_request(mddev, bio, r10_bio);
else
raid10_write_request(mddev, bio, r10_bio);
Then in raid10_write_request() during clustered resync:
if ((mddev_is_clustered(mddev) &&
mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
DEFINE_WAIT(w);
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
return;
}
And later during reshape:
if (bio->bi_opf & REQ_NOWAIT) {
allow_barrier(conf);
bio_wouldblock_error(bio);
return;
}
Since these paths return immediately, they leave the r10_bio allocated. Could
repeated REQ_NOWAIT rejections eventually exhaust conf->r10bio_pool, causing
subsequent allocations in __make_request() to hang indefinitely?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260622121312.1775322-1-chencheng@fnnas.com?part=3
^ permalink raw reply
* Re: [PATCH v5 1/3] md: suspend array when sync_action=reshape
From: sashiko-bot @ 2026-06-22 12:25 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260622121312.1775322-2-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Unmatched mddev_resume() on mddev_lock() failure causes mddev->suspended underflow.
- [High] The retry loop bypasses array re-suspension but leaves the suspended flag set, causing unprotected reshape and double-resume.
- [High] Suspending the array before calling flush_work() creates a circular memory reclaim deadlock.
Pre-existing issues:
- [High] Cross-task corruption of PF_MEMALLOC_NOIO flags when mddev_suspend() and mddev_resume() are interleaved.
--
commit ec573079e939aa92bfb3fd89247846ae57883ef2
Author: Chen Cheng <chencheng@fnnas.com>
md: suspend array when sync_action=reshape
This patch suspends the md array before starting a reshape operation to
prevent new requests from allocating objects from the old pool during
the transition. The array is then resumed on exit.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd5..e139f36e30b92 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5258,26 +5258,37 @@ static ssize_t
> action_store(struct mddev *mddev, const char *page, size_t len)
> {
> int ret;
> + bool suspended = false;
> enum sync_action action;
>
> if (!mddev->pers || !mddev->pers->sync_request)
> return -EINVAL;
>
> + action = md_sync_action_by_name(page);
> + if (action == ACTION_RESHAPE) {
> + ret = mddev_suspend(mddev, true);
[Severity: High]
This is a pre-existing issue, but does calling mddev_suspend() and
mddev_resume() from concurrent contexts risk corrupting the PF_MEMALLOC_NOIO
task flags?
When mddev_suspend() is called, it stores the task-specific NOIO flag into
the global mddev structure:
drivers/md/md.c:mddev_suspend() {
...
mddev->noio_flag = memalloc_noio_save();
...
}
If two tasks concurrently enter mddev_suspend(), the second task increments
the suspend counter without overwriting mddev->noio_flag. If the first task
then resumes (e.g., on a lock failure) it decrements the counter but returns
early because the array is still suspended:
drivers/md/md.c:__mddev_resume() {
if (mddev->suspended) {
mutex_unlock(&mddev->suspend_mutex);
return;
}
...
memalloc_noio_restore(mddev->noio_flag);
}
This prevents the first task from restoring its NOIO flag. When the second
task later resumes and drops the counter to 0, it incorrectly restores the
first task's flags onto its own context. Could this leave the first task
stuck in NOIO mode indefinitely?
> + if (ret)
> + return ret;
> + suspended = true;
> + }
> retry:
> if (work_busy(&mddev->sync_work))
> flush_work(&mddev->sync_work);
[Severity: High]
Can holding the array suspended while synchronously waiting for the worker
thread expose the worker's memory allocations to a circular deadlock?
If md_start_sync() allocates memory with GFP_KERNEL in the kworker thread
(for example, when calling md_register_thread()), it can trigger direct
memory reclaim. Because the worker thread did not transition the array to
suspended itself, it does not inherit the PF_MEMALLOC_NOIO flag.
Consequently, memory reclaim can issue I/O to the md device. Since the array
is now suspended, md_handle_request() will block this I/O indefinitely. The
worker thread hangs waiting for I/O, and action_store() hangs here in
flush_work() waiting for the worker thread, permanently deadlocking the
array. Is it safe to suspend the array before flushing the work?
>
> ret = mddev_lock(mddev);
> - if (ret)
> + if (ret) {
> + mddev_resume(mddev);
> return ret;
> + }
[Severity: High]
Does this unconditional mddev_resume() risk underflowing the mddev->suspended
counter if the array was not actually suspended?
If action_store() is called with an action other than ACTION_RESHAPE, the
array is not suspended. If the subsequent mddev_lock() fails (e.g., if it is
interrupted by a signal), the error path unconditionally calls mddev_resume().
Since the array was not suspended, __mddev_resume() decrements
mddev->suspended from 0 to -1. Because mddev->suspended is a signed integer
and -1 evaluates to true, subsequent calls to mddev_suspend() will see
mddev->suspended != 0, increment it back to 0, and return immediately without
actually pausing I/O. Could this permanently break the suspension mechanism
for the array?
>
> if (work_busy(&mddev->sync_work)) {
> - mddev_unlock(mddev);
> + if (suspended)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
> goto retry;
> }
[Severity: High]
Will the retry loop bypass array re-suspension while leaving the local
suspended flag set to true?
If work_busy(&mddev->sync_work) is true, the code unlocks and resumes the
array, then jumps back to the retry label. However, the retry label is placed
after the mddev_suspend() block.
The code will not re-suspend the array, but the local 'suspended' boolean
remains true. As a result, the reshape operation proceeds on an active,
unsuspended array. Furthermore, when the function exits, it will call
mddev_unlock_and_resume() again because 'suspended' is true, leading to a
double-resume and another mddev->suspended underflow.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260622121312.1775322-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* [PATCH v5 2/3] md/raid10: resize r10bio_pool for reshape
From: Chen Cheng @ 2026-06-22 12:13 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
In-Reply-To: <20260622121312.1775322-1-chencheng@fnnas.com>
From: Chen Cheng <chencheng@fnnas.com>
When reshape changes raid_disks, the pool must also switch to new geometry
object size.
Allocate a new geometry size pool and replace the old.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid10.c | 44 +++++++++++++++++++++++++++++++++-----------
drivers/md/raid10.h | 2 +-
2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index cee5a253a281..d740744a9746 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -101,14 +101,25 @@ static void end_reshape(struct r10conf *conf);
static inline struct r10bio *get_resync_r10bio(struct bio *bio)
{
return get_resync_pages(bio)->raid_bio;
}
-static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
+static inline int calc_r10bio_size(unsigned int raid_disks)
{
- struct r10conf *conf = data;
- int size = offsetof(struct r10bio, devs[conf->geo.raid_disks]);
+ return offsetof(struct r10bio, devs[raid_disks]);
+}
+
+static mempool_t *create_r10bio_pool(unsigned int raid_disks)
+{
+ int size = calc_r10bio_size(raid_disks);
+
+ return mempool_create_kmalloc_pool(NR_RAID_BIOS, size);
+}
+
+static struct r10bio *alloc_r10bio(unsigned int raid_disks, gfp_t gfp_flags)
+{
+ int size = calc_r10bio_size(raid_disks);
/* allocate a r10bio with room for raid_disks entries in the
* bios array */
return kzalloc(size, gfp_flags);
}
@@ -135,11 +146,11 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
struct bio *bio;
int j;
int nalloc, nalloc_rp;
struct resync_pages *rps;
- r10_bio = r10bio_pool_alloc(gfp_flags, conf);
+ r10_bio = alloc_r10bio(conf->geo.raid_disks, gfp_flags);
if (!r10_bio)
return NULL;
if (test_bit(MD_RECOVERY_SYNC, &conf->mddev->recovery) ||
test_bit(MD_RECOVERY_RESHAPE, &conf->mddev->recovery))
@@ -275,11 +286,11 @@ static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
static void free_r10bio(struct r10bio *r10_bio)
{
struct r10conf *conf = r10_bio->mddev->private;
put_all_bios(conf, r10_bio);
- mempool_free(r10_bio, &conf->r10bio_pool);
+ mempool_free(r10_bio, conf->r10bio_pool);
}
static void put_buf(struct r10bio *r10_bio)
{
struct r10conf *conf = r10_bio->mddev->private;
@@ -1537,11 +1548,11 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
{
struct r10conf *conf = mddev->private;
struct r10bio *r10_bio;
- r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
+ r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
r10_bio->master_bio = bio;
r10_bio->sectors = sectors;
r10_bio->mddev = mddev;
@@ -1729,11 +1740,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
last_stripe_index *= geo->far_copies;
end_disk_offset = (bio_end & geo->chunk_mask) +
(last_stripe_index << geo->chunk_shift);
retry_discard:
- r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
+ r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
r10_bio->mddev = mddev;
r10_bio->state = 0;
r10_bio->sectors = 0;
r10_bio->read_slot = -1;
memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * geo->raid_disks);
@@ -3830,11 +3841,11 @@ static int setup_geo(struct geom *geo, struct mddev *mddev, enum geo_type new)
static void raid10_free_conf(struct r10conf *conf)
{
if (!conf)
return;
- mempool_exit(&conf->r10bio_pool);
+ mempool_destroy(conf->r10bio_pool);
kfree(conf->mirrors);
kfree(conf->mirrors_old);
kfree(conf->mirrors_new);
safe_put_page(conf->tmppage);
bioset_exit(&conf->bio_split);
@@ -3877,13 +3888,12 @@ static struct r10conf *setup_conf(struct mddev *mddev)
if (!conf->tmppage)
goto out;
conf->geo = geo;
conf->copies = copies;
- err = mempool_init(&conf->r10bio_pool, NR_RAID_BIOS, r10bio_pool_alloc,
- rbio_pool_free, conf);
- if (err)
+ conf->r10bio_pool = create_r10bio_pool(conf->geo.raid_disks);
+ if (!conf->r10bio_pool)
goto out;
err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
if (err)
goto out;
@@ -4373,10 +4383,11 @@ static int raid10_start_reshape(struct mddev *mddev)
struct geom new;
struct r10conf *conf = mddev->private;
struct md_rdev *rdev;
int spares = 0;
int ret;
+ mempool_t *new_pool = NULL;
if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
return -EBUSY;
if (setup_geo(&new, mddev, geo_start) != conf->copies)
@@ -4409,10 +4420,15 @@ static int raid10_start_reshape(struct mddev *mddev)
if (spares < mddev->delta_disks)
return -EINVAL;
conf->offset_diff = min_offset_diff;
+ if (mddev->delta_disks > 0) {
+ new_pool = create_r10bio_pool(new.raid_disks);
+ if (!new_pool)
+ return -ENOMEM;
+ }
spin_lock_irq(&conf->device_lock);
if (conf->mirrors_new) {
memcpy(conf->mirrors_new, conf->mirrors,
sizeof(struct raid10_info)*conf->prev.raid_disks);
smp_mb();
@@ -4509,10 +4525,14 @@ static int raid10_start_reshape(struct mddev *mddev)
mddev->degraded = calc_degraded(conf);
spin_unlock_irq(&conf->device_lock);
mddev->raid_disks = conf->geo.raid_disks;
mddev->reshape_position = conf->reshape_progress;
set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
+ if (new_pool) {
+ mempool_destroy(conf->r10bio_pool);
+ conf->r10bio_pool = new_pool;
+ }
clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
@@ -4531,10 +4551,12 @@ static int raid10_start_reshape(struct mddev *mddev)
smp_wmb();
conf->reshape_progress = MaxSector;
conf->reshape_safe = MaxSector;
mddev->reshape_position = MaxSector;
spin_unlock_irq(&conf->device_lock);
+ if (new_pool)
+ mempool_destroy(new_pool);
return ret;
}
/* Calculate the last device-address that could contain
* any block from the chunk that includes the array-address 's'
diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
index ec79d87fb92f..b711626a5db7 100644
--- a/drivers/md/raid10.h
+++ b/drivers/md/raid10.h
@@ -85,11 +85,11 @@ struct r10conf {
int have_replacement; /* There is at least one
* replacement device.
*/
wait_queue_head_t wait_barrier;
- mempool_t r10bio_pool;
+ mempool_t *r10bio_pool;
mempool_t r10buf_pool;
struct page *tmppage;
struct bio_set bio_split;
/* When taking over an array from a different personality, we store
--
2.54.0
^ permalink raw reply related
* [PATCH v5 1/3] md: suspend array when sync_action=reshape
From: Chen Cheng @ 2026-06-22 12:13 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
In-Reply-To: <20260622121312.1775322-1-chencheng@fnnas.com>
From: Chen Cheng <chencheng@fnnas.com>
raid10 needs to resize/swap r10bio_pool when reshape changes
raid_disks, and, don't let new requests keep allocating r10bio
objects from the old pool while that transition is in progress.
suspend and lock array before mddev_start_reshape(), and resume
it on exit.
Other sync_action ops are unchanged.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/md.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 096bb64e87bd..e139f36e30b9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5256,30 +5256,41 @@ static int mddev_start_reshape(struct mddev *mddev)
static ssize_t
action_store(struct mddev *mddev, const char *page, size_t len)
{
int ret;
+ bool suspended = false;
enum sync_action action;
if (!mddev->pers || !mddev->pers->sync_request)
return -EINVAL;
+ action = md_sync_action_by_name(page);
+ if (action == ACTION_RESHAPE) {
+ ret = mddev_suspend(mddev, true);
+ if (ret)
+ return ret;
+ suspended = true;
+ }
retry:
if (work_busy(&mddev->sync_work))
flush_work(&mddev->sync_work);
ret = mddev_lock(mddev);
- if (ret)
+ if (ret) {
+ mddev_resume(mddev);
return ret;
+ }
if (work_busy(&mddev->sync_work)) {
- mddev_unlock(mddev);
+ if (suspended)
+ mddev_unlock_and_resume(mddev);
+ else
+ mddev_unlock(mddev);
goto retry;
}
- action = md_sync_action_by_name(page);
-
/* TODO: mdadm rely on "idle" to start sync_thread. */
if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
switch (action) {
case ACTION_FROZEN:
md_frozen_sync_thread(mddev);
@@ -5344,11 +5355,14 @@ action_store(struct mddev *mddev, const char *page, size_t len)
md_wakeup_thread(mddev->thread);
sysfs_notify_dirent_safe(mddev->sysfs_action);
ret = len;
out:
- mddev_unlock(mddev);
+ if (suspended)
+ mddev_unlock_and_resume(mddev);
+ else
+ mddev_unlock(mddev);
return ret;
}
static struct md_sysfs_entry md_scan_mode =
__ATTR_PREALLOC(sync_action, S_IRUGO|S_IWUSR, action_show, action_store);
--
2.54.0
^ permalink raw reply related
* [PATCH v5 0/3] md/raid10: fix r10bio width mismatches across reshape
From: Chen Cheng @ 2026-06-22 12:13 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
Hi,
This series fixes slab out-of-bounds accesses in raid10 when reshape changes
the number of raid disks while regular I/O is still reusing r10bio objects
allocated under the previous geometry.
The bug is reproducible with a simple 4-disk to 5-disk reshape under write
load, for example:
mdadm -C /dev/md777 -l10 -n4 /dev/sda /dev/sdb /dev/sdc /dev/sdd
mkfs.ext4 /dev/md777
mount /dev/md777 /mnt/test
fsstress -d /mnt/test -n 24000 -p 8 -l 24 &
mdadm /dev/md777 --add /dev/sde
mdadm --grow /dev/md777 --raid-devices=5 \
--backup-file=/tmp/md-reshape-backup
kcsan report:
BUG: KASAN: slab-out-of-bounds in free_r10bio+0x1c4/0x260 [raid10]
Read of size 8 at addr ffff00008c2dfac8 by task ksoftirqd/0/15
free_r10bio
raid_end_bio_io
one_write_done
raid10_end_write_request
This series addresses the problem in three steps:
1. ensure the sync_action=reshape caller suspends and locks before start_reshape
2. covert the r10bio pool fixed-size from old geometry to new.
3. reorder r10bio free flow to avoid race when free r10bio.
Changes in v5(suggesst by yukuai):
- patch 2 simpify
- patch 3 use new way{reorder free r10bio flow} instead of
old way {bound reused r10bio devs[] walks by used_nr_devs}
Changes in v4:
- The sync_action=reshape path, caller now invokes
mddev_suspend_and_lock() before calling start_reshape()
- The md-cluster and dm-raid paths are unchanged, that is reach
start_reshape() with the mddev locked but without suspended.
Changes in v3:
- Replace freeze_array()/unfreeze_array() in raid10_start_reshape() with
mddev_suspend_and_lock_nointr()/mddev_unlock_and_resume(). freeze_array()
returns when nr_pending == nr_queued, which still allows retry-list items
to hold pool objects; mddev_suspend() provides the correct upper-layer
quiesce interface. (Suggested by Yu Kuai)
Changes in v2:
- add this cover letter
- convert r10bio_pool to a fixed-size kmalloc mempool
- rebuild r10bio_pool inside the freeze window before switching live reshape
geometry
- switch raid10_quiesce() to freeze_array()/unfreeze_array()
Testing:
- reproduced the original KASAN slab-out-of-bounds on 4-disk -> 5-disk
raid10 reshape with fsstress
- verified that this series fixes that reproducer
- exercised the 5-disk -> 4-disk reshape direction as well
Thanks,
Chen Cheng
Chen Cheng (3):
md: suspend array before raid10 reshape via sync_action
md/raid10: make r10bio_pool use fixed-size objects
md/raid10: bound reused r10bio devs[] walks by used_nr_devs
drivers/md/md.c | 22 ++++++++++++++----
drivers/md/raid10.c | 56 +++++++++++++++++++++++++++++++++------------
drivers/md/raid10.h | 4 +++-
3 files changed, 61 insertions(+), 21 deletions(-)
--
2.54.0
^ permalink raw reply
* [PATCH v5 3/3] md/raid10: free r10bio before ending master_bio in raid_end_bio_io()
From: Chen Cheng @ 2026-06-22 12:13 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
In-Reply-To: <20260622121312.1775322-1-chencheng@fnnas.com>
From: Chen Cheng <chencheng@fnnas.com>
origin flow:
bio_endio(master_bio); /* may drop active_io to zero */
allow_barrier(conf);
free_r10bio(r10_bio); /* reads conf->geo, returns to pool */
one scenario is:
CPU A (softirq, raid_end_bio_io) CPU B (action_store) --> reshape
================================ ===============================
bio_endio(master_bio)
md_end_clone_io
percpu_ref_put -> 0
wait_event wakeup, and,
mddev_suspend return
raid10_start_reshape:
setup_geo(&conf->geo, new)
...
mempool_destroy(old_pool)
conf->r10bio_pool = new_pool
allow_barrier(conf)
free_r10bio(r10_bio)
put_all_bios:
for (i=0; i<conf->geo.raid_disks; i++)
==> old obj, new geo, OOB
mempool_free(r10_bio, conf->r10bio_pool)
==> old-geometry obj freed into new pool
so .. fix by reorder the flow:
free_r10bio(r10_bio)
allow_barrier(conf)
bio_endio(master_io)
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid10.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d740744a9746..a4642c903b20 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -330,24 +330,27 @@ static void reschedule_retry(struct r10bio *r10_bio)
*/
static void raid_end_bio_io(struct r10bio *r10_bio)
{
struct bio *bio = r10_bio->master_bio;
struct r10conf *conf = r10_bio->mddev->private;
+ bool returned = test_and_set_bit(R10BIO_Returned, &r10_bio->state);
+ blk_status_t status = test_bit(R10BIO_Uptodate, &r10_bio->state)
+ ? BLK_STS_OK : BLK_STS_IOERR;
- if (!test_and_set_bit(R10BIO_Returned, &r10_bio->state)) {
- if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
- bio->bi_status = BLK_STS_IOERR;
- bio_endio(bio);
- }
+ put_all_bios(conf, r10_bio);
+ mempool_free(r10_bio, conf->r10bio_pool);
/*
* Wake up any possible resync thread that waits for the device
* to go idle.
*/
allow_barrier(conf);
- free_r10bio(r10_bio);
+ if (!returned) {
+ bio->bi_status = status;
+ bio_endio(bio);
+ }
}
/*
* Update disk head position estimator based on IRQ completion info.
*/
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Dan Carpenter @ 2026-06-22 11:38 UTC (permalink / raw)
To: Sajal Gupta
Cc: linux-raid, song, linux-kernel, skhan, me, linux-kernel-mentees
In-Reply-To: <20260622112855.42685-1-sajal2005gupta@gmail.com>
On Mon, Jun 22, 2026 at 04:58:55PM +0530, Sajal Gupta wrote:
> > Heh. Yeah... There is not a chance I would merge a patch like this
> > without testing.
>
> > It also really feels like an AI patch and you're supposed to say when you
> > use AI to generate patches.
>
> I wrote the code myself. I did use Grammarly to help with my grammar and
> phrasing.
>
> I'll send a v2 with just the break fix.
Thanks.
You're right that calling atomic_dec() in a loop seems pretty suspect
as a design... But cleaning it up is risky as well.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Sajal Gupta @ 2026-06-22 11:28 UTC (permalink / raw)
To: error27; +Cc: linux-raid, song, linux-kernel, skhan, me, linux-kernel-mentees
In-Reply-To: <ajkVDGAdveCH_urr@stanley.mountain>
> Heh. Yeah... There is not a chance I would merge a patch like this
> without testing.
> It also really feels like an AI patch and you're supposed to say when you
> use AI to generate patches.
I wrote the code myself. I did use Grammarly to help with my grammar and
phrasing.
I'll send a v2 with just the break fix.
Thanks,
Sajal
^ permalink raw reply
* Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs
From: Chen Cheng @ 2026-06-22 11:13 UTC (permalink / raw)
To: yukuai, linux-raid; +Cc: linux-kernel
In-Reply-To: <a2f3a5da-71c3-4cff-ba97-db39e3ec4964@fygo.io>
在 2026/6/22 15:53, yu kuai 写道:
> Hi，
>
> 在 2026/6/22 14:34, Chen Cheng 写道:
>> The true dangerous is in-flight r10bio which use old geometry.
>
> You didn't explain why there can be inflight r10bio when array is suspended,
> if this can happen, this is the root cause need to be fixed.
>
one scenario is .
CPU A (softirq, raid_end_bio_io) CPU B (action_store)
================================ ===============================
bio_endio(master_bio)
md_end_clone_io
percpu_ref_put → 0
wait_event wakeup,
mddev_suspend return
raid10_start_reshape:
setup_geo(&conf->geo, new)
...
mempool_destroy(old_pool)
conf->r10bio_pool = new_pool
allow_barrier(conf)
free_r10bio(r10_bio)
put_all_bios:
for (i=0; i<conf->geo.raid_disks; i++)
==> old obj, new geo, OOB
mempool_free(r10_bio, conf->r10bio_pool)
==> old geo obj free into new pool
The fix, as your advise , raid_end_bio_io:
1. put_all_bios, mempool_free
2. bio_endio if returned
3. allow_barrier
would be replace with patch 03.
^ permalink raw reply
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Dan Carpenter @ 2026-06-22 10:57 UTC (permalink / raw)
To: Sajal Gupta
Cc: linux-raid, song, yukuai3, tomasz.majchrzak, linux-kernel, skhan,
me, linux-kernel-mentees
In-Reply-To: <20260622102859.38034-1-sajal2005gupta@gmail.com>
On Mon, Jun 22, 2026 at 03:58:58PM +0530, Sajal Gupta wrote:
> Hi Dan,
>
> > Have you tested this at all because it doesn't seem at all correct to
> > me...
>
> I have only done compile test, sorry, I forgot to mention that.
>
> > How I imagined this would work would be:
> > patch 1: add a break statement to fix the use after free
> > patch 2: s/atomic_t/recount_t/
>
> > The difference between atomic_t and refcount_t is that refount_t warns
> > about overflows and underflows.
>
> I did it like this because that is the pattern mostly used in the codebase.
> Simple s/atomic_t/recount_t/ would have
> refcount_set(&io->pending_flushes, 0) in init
> and then later refcount_set(&io->pending_flushes, raid_disks) in
> ppl_do_flush, which is not the usual way. I am treating it as a proper reference
> counter rather than a mechanical type swap.
>
> If that is too invasive, I’ll rework it into the break fix first, and do
> s/atomic_t/recount_t cleanup separately.
Heh. Yeah... There is not a chance I would merge a patch like this
without testing.
It also really feels like an AI patch and you're supposed to say when you
use AI to generate patches.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Sajal Gupta @ 2026-06-22 10:28 UTC (permalink / raw)
To: error27
Cc: linux-raid, song, yukuai3, tomasz.majchrzak, linux-kernel, skhan,
me, linux-kernel-mentees
In-Reply-To: <ajj1yljwrUk0bZwp@stanley.mountain>
Hi Dan,
> Have you tested this at all because it doesn't seem at all correct to
> me...
I have only done compile test, sorry, I forgot to mention that.
> How I imagined this would work would be:
> patch 1: add a break statement to fix the use after free
> patch 2: s/atomic_t/recount_t/
> The difference between atomic_t and refcount_t is that refount_t warns
> about overflows and underflows.
I did it like this because that is the pattern mostly used in the codebase.
Simple s/atomic_t/recount_t/ would have
refcount_set(&io->pending_flushes, 0) in init
and then later refcount_set(&io->pending_flushes, raid_disks) in
ppl_do_flush, which is not the usual way. I am treating it as a proper reference
counter rather than a mechanical type swap.
If that is too invasive, I’ll rework it into the break fix first, and do
s/atomic_t/recount_t cleanup separately.
Thanks,
Sajal
^ permalink raw reply
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Dan Carpenter @ 2026-06-22 8:43 UTC (permalink / raw)
To: Sajal Gupta
Cc: linux-raid, song, yukuai3, tomasz.majchrzak, linux-kernel, skhan,
me, linux-kernel-mentees
In-Reply-To: <ajj1WdhoW-Y3GE4Q@stanley.mountain>
On Mon, Jun 22, 2026 at 11:42:01AM +0300, Dan Carpenter wrote:
> On Mon, Jun 22, 2026 at 01:34:32PM +0530, Sajal Gupta wrote:
> > The old atomic_t based counter allowed ppl_do_flush() to continue using io
> > after it could already have been freed by ppl_io_unit_finished(), leading
> > to a use-after-free.
> >
> > Convert pending_flushes from atomic_t to refcount_t with a proper ownership
> > model. The creator holds a reference for the duration of ppl_do_flush(),
> > and each submitted flush bio holds a reference until its endio callback
> > runs. This makes the io lifetime explicit and removes the need for the
> > second loop in ppl_do_flush().
> >
> > Fixes: 1532d9e87e8b ("raid5-ppl: PPL support for disks with write-back cache enabled")
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Closes: https://lore.kernel.org/all/ajJF2wKYWRk4GGCK@stanley.mountain/
> > Signed-off-by: Sajal Gupta <sajal2005gupta@gmail.com>
> > ---
>
> Have you tested this at all because it doesn't seem at all correct to
> me...
How I imagined this would work would be:
patch 1: add a break statement to fix the use after free
patch 2: s/atomic_t/recount_t/
The difference between atomic_t and refcount_t is that refount_t warns
about overflows and underflows.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Dan Carpenter @ 2026-06-22 8:42 UTC (permalink / raw)
To: Sajal Gupta
Cc: linux-raid, song, yukuai3, tomasz.majchrzak, linux-kernel, skhan,
me, linux-kernel-mentees
In-Reply-To: <20260622080656.22786-1-sajal2005gupta@gmail.com>
On Mon, Jun 22, 2026 at 01:34:32PM +0530, Sajal Gupta wrote:
> The old atomic_t based counter allowed ppl_do_flush() to continue using io
> after it could already have been freed by ppl_io_unit_finished(), leading
> to a use-after-free.
>
> Convert pending_flushes from atomic_t to refcount_t with a proper ownership
> model. The creator holds a reference for the duration of ppl_do_flush(),
> and each submitted flush bio holds a reference until its endio callback
> runs. This makes the io lifetime explicit and removes the need for the
> second loop in ppl_do_flush().
>
> Fixes: 1532d9e87e8b ("raid5-ppl: PPL support for disks with write-back cache enabled")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/all/ajJF2wKYWRk4GGCK@stanley.mountain/
> Signed-off-by: Sajal Gupta <sajal2005gupta@gmail.com>
> ---
Have you tested this at all because it doesn't seem at all correct to
me...
regards,
dan carpenter
^ permalink raw reply
* [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t
From: Sajal Gupta @ 2026-06-22 8:04 UTC (permalink / raw)
To: linux-raid, song
Cc: yukuai3, tomasz.majchrzak, linux-kernel, error27, skhan, me,
linux-kernel-mentees, Sajal Gupta
The old atomic_t based counter allowed ppl_do_flush() to continue using io
after it could already have been freed by ppl_io_unit_finished(), leading
to a use-after-free.
Convert pending_flushes from atomic_t to refcount_t with a proper ownership
model. The creator holds a reference for the duration of ppl_do_flush(),
and each submitted flush bio holds a reference until its endio callback
runs. This makes the io lifetime explicit and removes the need for the
second loop in ppl_do_flush().
Fixes: 1532d9e87e8b ("raid5-ppl: PPL support for disks with write-back cache enabled")
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/all/ajJF2wKYWRk4GGCK@stanley.mountain/
Signed-off-by: Sajal Gupta <sajal2005gupta@gmail.com>
---
drivers/md/raid5-ppl.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
index a70cbec12ed0..157a89edd9c8 100644
--- a/drivers/md/raid5-ppl.c
+++ b/drivers/md/raid5-ppl.c
@@ -145,7 +145,7 @@ struct ppl_io_unit {
struct list_head stripe_list; /* stripes added to the io_unit */
atomic_t pending_stripes; /* how many stripes not written to raid */
- atomic_t pending_flushes; /* how many disk flushes are in progress */
+ refcount_t pending_flushes; /* how many disk flushes are in progress */
bool submitted; /* true if write to log started */
@@ -249,7 +249,7 @@ static struct ppl_io_unit *ppl_new_iounit(struct ppl_log *log,
INIT_LIST_HEAD(&io->log_sibling);
INIT_LIST_HEAD(&io->stripe_list);
atomic_set(&io->pending_stripes, 0);
- atomic_set(&io->pending_flushes, 0);
+ refcount_set(&io->pending_flushes, 1);
bio_init(&io->bio, log->rdev->bdev, io->biovec, PPL_IO_INLINE_BVECS,
REQ_OP_WRITE | REQ_FUA);
@@ -599,7 +599,7 @@ static void ppl_flush_endio(struct bio *bio)
bio_put(bio);
- if (atomic_dec_and_test(&io->pending_flushes)) {
+ if (refcount_dec_and_test(&io->pending_flushes)) {
ppl_io_unit_finished(io);
md_wakeup_thread(conf->mddev->thread);
}
@@ -611,11 +611,8 @@ static void ppl_do_flush(struct ppl_io_unit *io)
struct ppl_conf *ppl_conf = log->ppl_conf;
struct r5conf *conf = ppl_conf->mddev->private;
int raid_disks = conf->raid_disks;
- int flushed_disks = 0;
int i;
- atomic_set(&io->pending_flushes, raid_disks);
-
for_each_set_bit(i, &log->disk_flush_bitmap, raid_disks) {
struct md_rdev *rdev;
struct block_device *bdev = NULL;
@@ -632,20 +629,18 @@ static void ppl_do_flush(struct ppl_io_unit *io)
GFP_NOIO, &ppl_conf->flush_bs);
bio->bi_private = io;
bio->bi_end_io = ppl_flush_endio;
+ refcount_inc(&io->pending_flushes);
pr_debug("%s: dev: %ps\n", __func__, bio->bi_bdev);
submit_bio(bio);
- flushed_disks++;
}
}
log->disk_flush_bitmap = 0;
- for (i = flushed_disks ; i < raid_disks; i++) {
- if (atomic_dec_and_test(&io->pending_flushes))
- ppl_io_unit_finished(io);
- }
+ if (refcount_dec_and_test(&io->pending_flushes))
+ ppl_io_unit_finished(io);
}
static inline bool ppl_no_io_unit_submitted(struct r5conf *conf,
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs
From: yu kuai @ 2026-06-22 7:53 UTC (permalink / raw)
To: Chen Cheng, linux-raid; +Cc: linux-kernel, yukuai
In-Reply-To: <f877d9f4-4912-4331-8c14-e7726adf24bc@fnnas.com>
Hi，
在 2026/6/22 14:34, Chen Cheng 写道:
> The true dangerous is in-flight r10bio which use old geometry.
You didn't explain why there can be inflight r10bio when array is suspended,
if this can happen, this is the root cause need to be fixed.
--
Thanks,
Kuai
^ 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