Re: [PATCH v3 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] A concurrent promotion or removal of a disk causes a NULL pointer dereference in the err_handle cleanup path.
--

--- Patch [3]: [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting ---
commit 39f996c40527039ea2fa463cb02f7e201f0a4e7d
Author: Abd-Alrhman Masalkhi <[email protected]>

md/raid10: consistently fail atomic writes that require splitting

RAID10 currently handles one badblock path explicitly by failing atomic
writes with EIO. However, another badblock path can also reduce the
writable range and force the bio through bio_submit_split_bioset(),
which implicitly completes the bio with EINVAL. This patch explicitly 
fails atomic writes in the common split check instead.

Link: https://lore.kernel.org/linux-raid/[email protected]/

> 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 lead to a NULL pointer dereference
if a concurrent disk removal or promotion occurs?

In the err_handle cleanup path, the replacement pointer is locklessly
re-read from the array configuration:

err_handle:
	for (k = 0;  k < i; k++) {
		int d = r10_bio->devs[k].devnum;
		struct md_rdev *rdev = conf->mirrors[d].rdev;
		struct md_rdev *rrdev = conf->mirrors[d].replacement;

If raid10_write_request() executes without holding rcu_read_lock() or
reconfig_mutex (since it can sleep), it might increment nr_pending on the
active replacement disk and set r10_bio->devs[i].repl_bio = bio.

Concurrently, if raid10_remove_disk() promotes the replacement disk, it sets
conf->mirrors[d].replacement = NULL. 

When this goto is hit because an atomic write needs splitting, the lockless
re-read will fetch NULL for rrdev. Since r10_bio->devs[k].repl_bio is
non-NULL, the code calls rdev_dec_pending(rrdev, mddev), which will
dereference the NULL pointer.

>  		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