Re: [PATCH v2 6/7] md/raid1,raid10: skip futile retries on P2PDMA mapping failures

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

Pre-existing issues:
- [High] Forcing REQ_OP_WRITE on failed dataless bios in narrow_write_error() causes a NULL pointer dereference.
- [High] narrow_write_error drops critical bio flags (such as REQ_FUA or REQ_PREFLUSH) when retrying failed writes.
- [High] Possible NULL pointer dereference when logging unrecoverable read errors in raid1.
--

--- Patch [6]: [PATCH v2 6/7] md/raid1,raid10: skip futile retries on P2PDMA mapping failures ---
commit ff103403bf433f1316745e0b4415b739bb72c8d1
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: skip futile retries on P2PDMA mapping failures

This commit adds tracking for P2PDMA masters via a state bit to handle peer
device memory mapping failures explicitly. It prevents kicking healthy mirrors
and short-circuits futile retry loops by treating unroutable P2PDMA bio
failures as a property of the topology rather than the medium.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -1378,6 +1388,8 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>  	else
>  		init_r1bio(r1_bio, mddev, bio);
>  	r1_bio->sectors = max_read_sectors;
> +	if (md_bio_is_p2pdma(bio))
> +		set_bit(R1BIO_P2PDMA, &r1_bio->state);
>  
>  	/*
>  	 * make_request() can abort the operation when read-ahead is being

[Severity: High]
This is a pre-existing issue, but could the error logging in
raid1_read_request() result in a NULL pointer dereference?

When an unrecoverable read error occurs, handle_read_error() drops its
active reference to the failing device via rdev_dec_pending() and then calls
raid1_read_request() to attempt a retry.

If read_balance() cannot find an alternative mirror, the error logging uses
conf->mirrors[r1_bio->read_disk].rdev->bdev:

    rdisk = read_balance(conf, r1_bio, &max_sectors);
    if (rdisk < 0) {
        if (r1bio_existed)
            pr_crit_ratelimited("md/raid1:%s: %pg: ...\n",
                        mdname(mddev),
                        conf->mirrors[r1_bio->read_disk].rdev->bdev,
                        r1_bio->sector);
        raid_end_bio_io(r1_bio);
        return;
    }

Since the reference to this disk was just dropped and the RCU read lock
is not held, is it possible for a concurrent hot_remove_disk operation to set
rdev to NULL, leaving this bdev access unprotected?

[ ... ]
> @@ -2539,7 +2553,7 @@ static void fix_read_error(struct r1conf *conf, struct r1bio *r1_bio)
>  	}
>  }
>  
> -static void narrow_write_error(struct r1bio *r1_bio, int i)
> +static void narrow_write_error(struct r1bio *r1_bio, int i, bool coarse)
>  {
>  	struct mddev *mddev = r1_bio->mddev;

[Severity: High]
This isn't a bug introduced by this patch, but does the error retry path
in narrow_write_error() inadvertently corrupt the bio operation and flags?

When cloning a failed bio for retrying, the code unconditionally overwrites
the bi_opf field:

    wbio->bi_opf = REQ_OP_WRITE;

If the failed master bio was a dataless operation (such as REQ_OP_WRITE_ZEROES),
the operation override happens before bio_trim(). For multi-block requests,
bio_trim() calls bio_advance(). Because the bio is now incorrectly seen as a
standard write, bio_no_advance_iter() returns false, and the block layer
attempts to advance the NULL bi_io_vec, which will cause a kernel panic.

Additionally, the direct assignment of REQ_OP_WRITE completely clobbers
pre-existing request flags. If a flush or FUA write fails and is retried,
it seems it will be submitted as a normal asynchronous write. If the retry
succeeds, data durability guarantees might be violated since the data could
only reside in the volatile disk cache.

[ ... ]
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -2786,7 +2803,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
>  	}
>  }
>  
> -static void narrow_write_error(struct r10bio *r10_bio, int i)
> +static void narrow_write_error(struct r10bio *r10_bio, int i, bool coarse)
>  {
>  	struct bio *bio = r10_bio->master_bio;

[Severity: High]
This is a pre-existing issue, but does narrow_write_error() similarly
corrupt dataless bios and drop durability flags here?

Similar to raid1, the clone unconditionally overwrites bi_opf:

    wbio->bi_opf = REQ_OP_WRITE;

While bio_trim() happens before the override in raid10 (avoiding the
iterator crash), it appears to submit the modified REQ_OP_WRITE bio
without a payload to lower drivers, which could crash during DMA mapping.

It also drops critical bio modifiers like REQ_FUA or REQ_PREFLUSH, which
could lead to silent data corruption on power failure if a synchronous write
encounters a transient error.

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