[PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Dan Carpenter]

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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Dan Carpenter]

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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Sajal Gupta]

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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Dan Carpenter]

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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Sajal Gupta]

> 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

Re: [PATCH] md/raid5-ppl: convert pending_flushes from atomic_t to refcount_t

[Dan Carpenter]

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