Re: [PATCH v4 8/9] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA

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

Pre-existing issues:
- [Critical] NULL pointer dereference when a read error occurs and the failing disk is concurrently removed.
- [High] Barrier count underflow leading to array deadlock for REQ_NOWAIT retries.
--

--- Patch [8]: [PATCH v4 8/9] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA ---
commit 64027366aa2bf452a9087c9fd113c84027594e8b
Author: Logan Gunthorpe <[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. This skips the
read-error machinery, avoiding the eviction of healthy members under a P2P
read workload.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 917d694ef401a..76a1426e64f02 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -2675,7 +2677,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 || p2pdma_error) {
>  		r1_bio->bios[r1_bio->read_disk] = IO_BLOCKED;
>  	} else if (test_bit(FailFast, &rdev->flags)) {
>  		md_error(mddev, rdev);

[Severity: Critical]
This isn't a bug introduced by this patch, but can we hit a NULL pointer
dereference if the failing disk is concurrently removed?

When a read error occurs, the bio is routed here to handle_read_error().
This function drops the pending count via rdev_dec_pending(rdev), which
could allow a concurrent mdadm --remove to succeed and set
conf->mirrors[r1_bio->read_disk].rdev to NULL.

The retry is then submitted via raid1_read_request(). If all healthy disks
are blocked or unavailable, read_balance() can return -1, triggering this
logging path in raid1_read_request():

raid1_read_request() {
    ...
    if (rdisk < 0) {
        ...
        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,
                            ...);
    ...
}

Since rdev is now NULL, will this dereference rdev->bdev and panic the
kernel? For comparison, raid10_read_request() appears to correctly check
if rdev is NULL before logging in this scenario.

[Severity: High]
This is a pre-existing issue, but could this error path lead to a barrier
count underflow and array deadlock when retrying REQ_NOWAIT requests?

If a REQ_NOWAIT read fails and reaches handle_read_error(), it retries by
calling raid1_read_request() with the existing r1_bio.

Inside raid1_read_request(), wait_read_barrier() can return false due to
the nowait flag and the array being concurrently frozen, which leaves
nr_pending unchanged. However, raid1_read_request() then calls
raid_end_bio_io():

raid1_read_request() {
    ...
    if (r1bio_existed) {
        set_bit(R1BIO_Returned, &r1_bio->state);
        raid_end_bio_io(r1_bio);
    }
    ...
}

This calls allow_barrier(), decrementing the barrier count. Control then
returns to handle_read_error(), which ends with:

handle_read_error() {
    ...
    raid1_read_request(mddev, bio, r1_bio->sectors, r1_bio);
    allow_barrier(conf, sector);
}

Does this second allow_barrier() decrement the count again for a single I/O,
causing a net barrier underflow that blocks future raise_barrier() calls
and deadlocks the array?

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