Re: [PATCH v3 6/8] md/raid1: skip futile retries on P2PDMA mapping failures

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

New issues:
- [High] Coarse retries for persistent P2PDMA mapping failures exhaust the array's badblock list, forcing healthy device eviction.

Pre-existing issues:
- [High] Data corruption and kernel panics result from wbio->bi_opf = REQ_OP_WRITE; stripping critical flags and modifying bodiless bios.
- [High] A NULL pointer dereference occurs on rdev->bdev during concurrent read errors and hot-removes.
--

--- Patch [6]: [PATCH v3 6/8] md/raid1: skip futile retries on P2PDMA mapping failures ---

commit 808452a00e434c54e8f26591e906bbf52dd9e7f0
Author: Mykola Marzhan [off-list ref]

md/raid1: skip futile retries on P2PDMA mapping failures

A member that cannot DMA-map peer memory fails the leg bio with
BLK_STS_TARGET.  The failure is a property of the peer/member
pairing -- retrying the same pages against the same member cannot
succeed, and there is nothing on the medium to repair. This patch
flags P2PDMA master bios with a new R1BIO_P2PDMA state bit at
submission to avoid evicting healthy members during unroutable I/O.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference?

When a read fails, handle_read_error() drops the reference to the rdev via
rdev_dec_pending(), which may trigger device removal if it's faulty. It then
calls raid1_read_request() to retry.

In raid1_read_request(), if read_balance() returns -1 (no other disks are
available), we hit a pr_crit_ratelimited statement:

    rdisk = read_balance(conf, r1_bio, &max_sectors);
    if (rdisk < 0) {
        /* couldn't find anywhere to read from */
        if (r1bio_existed)
            pr_crit_ratelimited("md/raid1:%s: %pg: unrecoverable I/O read error for block %llu\n",
                        mdname(mddev),
                        conf->mirrors[r1_bio->read_disk].rdev->bdev,
                        r1_bio->sector);

Is it possible that conf->mirrors[r1_bio->read_disk].rdev is NULL here due
to a concurrent hot-remove, causing a crash when dereferencing ->bdev?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does overwriting the operation entirely
strip critical upper durability flags here?

In narrow_write_error(), the cloned wbio is modified like this:

        wbio->bi_opf = REQ_OP_WRITE;

Could this strip flags like REQ_FUA or REQ_PREFLUSH, leading to potential
data loss on power failure? Furthermore, if a bodiless bio like
REQ_OP_WRITE_ZEROES is converted into a regular REQ_OP_WRITE, could this
cause a NULL pointer dereference in the block layer during segment mapping
(e.g., bio_for_each_segment) since it has no data vectors?

[Severity: High]
With the new coarse retry mechanism, what happens when persistent P2PDMA
mapping failures repeatedly occur?

If a P2PDMA write fails with BLK_STS_TARGET, we enter narrow_write_error()
with coarse = true. Later in this loop, if the retried coarse I/O also
fails, it adds a badblock:

        if (submit_bio_wait(wbio) &&
            !rdev_set_badblocks(rdev, sector, sectors, 0)) {

Since P2PDMA pairing failures are a persistent property of the peer/member
combination, repeated coarse retries will keep adding large badblock entries.

If this quickly exhausts the array's finite badblocks list, won't
rdev_set_badblocks() return 0, triggering a call to md_error() that forces
the eviction of a healthy device? Does this defeat the patch's purpose of
preventing healthy devices from being evicted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721174502.111503-1-mykola@meshstor.io?part=6