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 <[email protected]>

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.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 16b7465c233ac..6334032c4bb8e 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -1378,6 +1384,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 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?

[ ... ]
> @@ -2550,9 +2563,12 @@ static void narrow_write_error(struct r1bio *r1_bio, int i)
>  		block_sectors = roundup(1 << rdev->badblocks.shift, lbs);
>  
>  	sector = r1_bio->sector;
> -	sectors = ((sector + block_sectors)
> -		   & ~(sector_t)(block_sectors - 1))
> -		- sector;
> +	if (coarse)
> +		sectors = sect_to_write;
> +	else
> +		sectors = ((sector + block_sectors)
> +			   & ~(sector_t)(block_sectors - 1))
> +			- sector;
>  
>  	while (sect_to_write) {
>  		struct bio *wbio;

[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/[email protected]?part=6