Re: [PATCH v5 3/3] md/raid10: free r10bio before ending master_bio in raid_end_bio_io()

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 <[email protected]>

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/[email protected]?part=3