Re: [PATCH v4 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] Concurrent disk removal during a failed or split write causes a NULL pointer dereference in the error handling path.
--

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

md/raid10: consistently fail atomic writes that require splitting

This patch addresses atomic writes in RAID10 by checking if they require
splitting. If an atomic write would be split due to badblocks or other
limits, it now correctly fails the bio with EIO rather than allowing the
implicit EINVAL from bio_submit_split_bioset().

> 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 handling path
trigger a NULL pointer dereference if a disk removal happens concurrently?

If raid10_write_request() initiates an IO and skips a Faulty primary rdev,
it uses the replacement (setting repl_bio and incrementing rrdev->nr_pending).
If md_check_recovery() concurrently calls raid10_remove_disk() for the Faulty
primary, it can promote the replacement to primary and set p->replacement
to NULL.

When this atomic check fails, it jumps to the error unwinding loop which
does:

    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;
    }

Since replacement is now NULL, rrdev is NULL, and rdev_dec_pending() will
dereference it.

Looking at raid10_end_write_request(), it implements a fallback to handle
concurrent promotion by falling back to conf->mirrors[d].rdev when
replacement is NULL.

Does the error unwinding loop in raid10_write_request() need a similar
fallback?

> +
>  		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