Re: [PATCH v2 3/7] md/raid10: consistently fail atomic writes that require splitting

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `err_dec_pending` (formerly `err_handle`) cleanup path in `raid10_write_request` can trigger a NULL pointer dereference when a replacement device is promoted concurrently.
--

commit bc9d81fdb2c33f8c55baad27ab1eedf427ae25ed
Author: Abd-Alrhman Masalkhi <[email protected]>

md/raid10: consistently fail atomic writes that require splitting

RAID10 handles one badblock path explicitly by failing atomic writes with
EIO, but another path can also reduce the writable range and force the bio
through bio_submit_split_bioset(), completing it with EINVAL. This commit
centralizes the check to fail all atomic writes requiring splitting with
EIO.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index bd322eccdc3ff..3480fc7907f01 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
>  		r10_bio->sectors = max_sectors;
>  
>  	if (r10_bio->sectors < bio_sectors(bio)) {
> +		if (atomic)
> +			goto err_handle;
> +

[Severity: High]
This is a pre-existing issue, but can this jump to the error handler cause a
null pointer dereference if a replacement device is concurrently promoted?

In the error handler path, the code fetches the replacement device without
checking for null or falling back to the primary slot:

err_handle:
	for (k = 0;  k < i; k++) {
		int d = r10_bio->devs[k].devnum;
		struct md_rdev *rrdev = conf->mirrors[d].replacement;
		...
		if (r10_bio->devs[k].repl_bio) {
			rdev_dec_pending(rrdev, mddev);
			r10_bio->devs[k].repl_bio = NULL;
		}
	}

If raid10_remove_disk() runs concurrently, it can promote the replacement to
the primary slot and clear conf->mirrors[d].replacement:

raid10_remove_disk() {
	...
	WRITE_ONCE(*rdevp, NULL);
	if (p->replacement) {
		WRITE_ONCE(p->rdev, p->replacement);
		clear_bit(Replacement, &p->replacement->flags);
		WRITE_ONCE(p->replacement, NULL);
	}
	...
}

If this concurrent promotion happens, will rrdev be null here? It looks like
other paths such as raid10_end_write_request() safely handle this exact
race by falling back to rdev when replacement is null, but this cleanup
path does not.

>  		allow_barrier(conf);
>  		bio = bio_submit_split_bioset(bio, r10_bio->sectors,
>  					      &conf->bio_split);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3