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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `md_bio_p2pdma_mapping_error()` accesses `bio->bi_io_vec->bv_page` without a valid operation type guard, leading to a wild pointer dereference on data-less bios.
--

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

md/raid1,raid10: skip futile retries on P2PDMA mapping failures
    
This patch modifies raid1 and raid10 to explicitly handle P2PDMA mapping
failures (BLK_STS_TARGET). It avoids excessive per-block retries and
prevents healthy members from being evicted when peer memory cannot be
DMA-mapped.

> diff --git a/drivers/md/md.h b/drivers/md/md.h
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -40,6 +40,19 @@ static inline bool md_bio_is_p2pdma(struct bio *bio)
>  	       is_pci_p2pdma_page(bio->bi_io_vec->bv_page);
>  }
>  
> +/*
> + * True when a leg bio failed because its P2PDMA pages cannot be DMA-mapped
> + * to this member (BLK_STS_TARGET from blk_dma_map_iter_start()). Usable at
> + * completion time, unlike md_bio_is_p2pdma(): leg bios are clones sharing
> + * the master bio's bvec table, which outlives the leg's end_io, but their
> + * iterator is consumed by then, so bio_has_data() cannot gate the access.
> + */
> +static inline bool md_bio_p2pdma_mapping_error(struct bio *bio)
> +{
> +	return bio->bi_status == BLK_STS_TARGET && bio->bi_io_vec &&
> +	       is_pci_p2pdma_page(bio->bi_io_vec->bv_page);
> +}

[Severity: High]
Can this result in a wild pointer dereference for data-less bios?

Data-less bios (like REQ_OP_FLUSH) can legally be allocated with nr_vecs > 0,
such as when they are reused via bio_reset(). For these bios, bio->bi_io_vec
might be non-NULL while bv_page remains uninitialized.

If such a bio fails with BLK_STS_TARGET (for example, from an NVMe drive
returning NVME_SC_NS_NOT_READY), is it safe to access bv_page here and pass
it to is_pci_p2pdma_page() without a bio_has_data() guard?

[ ... ]

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2629,7 +2640,20 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>  			 * errors.
>  			 */
>  			fail = true;
> -			narrow_write_error(r1_bio, m);
> +			if (md_bio_p2pdma_mapping_error(r1_bio->bios[m]))

[Severity: High]
Will this error path unconditionally pass failed cloned bios, including
data-less ones like REQ_OP_FLUSH, to the mapping check above?

> +				/*
> +				 * A P2PDMA mapping failure fails the whole
> +				 * range identically, so narrowing block by

[ ... ]

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2948,7 +2971,23 @@ static void handle_write_completed(struct r10conf *conf, struct r10bio *r10_bio)
>  				rdev_dec_pending(rdev, conf->mddev);
>  			} else if (bio != NULL && bio->bi_status) {
>  				fail = true;
> -				narrow_write_error(r10_bio, m);
> +				if (md_bio_p2pdma_mapping_error(bio))

[Severity: High]
Similarly, does this RAID10 error path pass failed bios to the mapping
check without verifying if the operation actually carries data?

> +					/*
> +					 * A P2PDMA mapping failure fails
> +					 * the whole range identically, so
> +					 * narrowing block by block cannot

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