Re: [PATCH v5 08/10] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA

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

New issues:
- [High] Bypassing `freeze_array()` for `BLK_STS_P2PDMA` in `raid1.c` introduces a hard deadlock against concurrent array freezes.

Pre-existing issues:
- [High] Race condition leading to a NULL pointer dereference in `raid1_read_request()` when logging an unrecoverable read error.
- [High] Double barrier drop (`nr_pending` underflow) when a `REQ_NOWAIT` read fails while the array is frozen.
--

--- Patch [8]: [PATCH v5 08/10] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA ---
commit 39fd90f26512b119562fa95bb3bbc654fa9ed8a3
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA

A read that fails with BLK_STS_P2PDMA cannot succeed against that
member, so mark the leg IO_BLOCKED and let the retry redirect elsewhere.
Skip the read-error machinery: there is nothing on the medium to fix,
fix_read_error()'s probe reads into host pages and would "succeed", and
charging the read-error budget would evict a healthy member under a P2P
read workload.  FailFast eviction is skipped for the same reason -- the
request never reached the wire.

Fixes: 02666132403a ("md: propagate BLK_FEAT_PCI_P2PDMA from member devices to RAID device")
Assisted-by: Claude:claude-fable-5
Co-developed-by: Logan Gunthorpe <[email protected]>
Signed-off-by: Logan Gunthorpe <[email protected]>
Signed-off-by: Mykola Marzhan <[email protected]>

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 83ebf40237d57..16a47568679f0 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -2674,7 +2676,7 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
>  	 * read error.  This is all done synchronously while the array is
>  	 * frozen.
>  	 */
> -	if (mddev->ro) {
> +	if (mddev->ro || status == BLK_STS_P2PDMA) {
>  		r1_bio->bios[r1_bio->read_disk] = IO_BLOCKED;

[Severity: High]
Does skipping freeze_array() for P2PDMA errors here risk a hard deadlock?

If a concurrent thread calls freeze_array(conf, 0) during a management command,
it sets array_frozen = 1 and waits for get_unqueued_pending() == 0.

Meanwhile, this handle_read_error() thread bypasses freeze_array() and
proceeds to call raid1_read_request() while holding the original nr_pending
reference.

When raid1_read_request() calls wait_read_barrier():

drivers/md/raid1.c:raid1_read_request() {
    ...
    if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
    ...
}

And wait_read_barrier() sees the array is frozen:

drivers/md/raid1.c:wait_read_barrier() {
    ...
    if (unlikely(conf->array_frozen)) {
        ...
        wait_event_lock_irq_cmd(...)
    ...
}

The raid1d thread will block on array_frozen, but because it still holds its
nr_pending reference, get_unqueued_pending() will remain >= 1. The freezer
thread will wait forever for the reference to drop, and raid1d will wait
forever for the array to unfreeze.

>  	} else if (test_bit(FailFast, &rdev->flags)) {
>  		md_error(mddev, rdev);

[Severity: High]
This is a pre-existing issue, but does dropping the active reference to the
failing device later in handle_read_error() introduce a race condition with
concurrent disk removal?

Looking further down in handle_read_error():

    rdev_dec_pending(rdev, conf->mddev);
    sector = r1_bio->sector;
    ...
    raid1_read_request(mddev, bio, r1_bio->sectors, r1_bio);

Since raid1d doesn't hold reconfig_mutex, a concurrent raid1_remove_disk() can
observe nr_pending == 0 and clear the device pointer. If the subsequent call
to raid1_read_request() fails to find an alternative disk in read_balance(),
it attempts to log a critical error:

drivers/md/raid1.c:raid1_read_request() {
    ...
    if (r1_bio->read_disk < 0) {
        ...
        pr_crit_ratelimited("md/raid1:%s: %pg: unrecoverable I/O read...\n",
                            mdname(mddev),
                            conf->mirrors[r1_bio->read_disk].rdev->bdev,
                            ...);
    ...
}

Dereferencing conf->mirrors[r1_bio->read_disk].rdev->bdev when rdev has been
set to NULL would cause a fatal page fault.


[Severity: High]
This is also a pre-existing issue, but could the unconditional allow_barrier()
at the end of handle_read_error() lead to a double barrier drop if a
REQ_NOWAIT read fails while the array is frozen?

Looking at the end of the function:

    raid1_read_request(mddev, bio, r1_bio->sectors, r1_bio);
    allow_barrier(conf, sector);

If wait_read_barrier() inside raid1_read_request() encounters a frozen array
and returns false due to REQ_NOWAIT, it does not increment nr_pending:

drivers/md/raid1.c:raid1_read_request() {
    ...
    if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
        bio_wouldblock_error(bio);
        raid_end_bio_io(r1_bio);
        return;
    ...
}

raid_end_bio_io() internally calls allow_barrier() to drop the original
reference. After raid1_read_request() returns, handle_read_error() will
unconditionally execute this second allow_barrier(conf, sector).

This would cause nr_pending to underflow, leading to a permanent deadlock on
any subsequent array freeze since raise_barrier() will block forever waiting
for nr_pending == 0.

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