[bug report] raid5-ppl: PPL support for disks with write-back cache enabled

[bug report] raid5-ppl: PPL support for disks with write-back cache enabled

[Dan Carpenter]

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

Re: [bug report] raid5-ppl: PPL support for disks with write-back cache enabled

[Dan Carpenter]

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