Re: [PATCH 3/6] md/raid1: don't use write-behind for P2PDMA bios

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

Pre-existing issues:
- [Critical] P2PDMA bios bypass write-behind but fail to serialize against outstanding write-behind I/O, leading to silent data corruption on write-mostly devices.
--

--- Patch [3]: [PATCH 3/6] md/raid1: don't use write-behind for P2PDMA bios ---
commit bbb98e00f3bb1bd5712cfce20170706425ddb589
Author: Mykola Marzhan <[email protected]>

md/raid1: don't use write-behind for P2PDMA bios

alloc_behind_master_bio() copies the bio's data with bio_copy_data(),
a CPU copy. P2PDMA pages are peer device (BAR) memory; generic code
must not assume CPU load/store access to them is safe or fast on
every architecture, and bouncing peer memory through the CPU defeats
the point of a peer-to-peer transfer.

Skip write-behind for P2PDMA bios: they are written directly to all
members, including write-mostly ones.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index afe2ca96ad8c2..57f64e8901024 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1575,9 +1575,12 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		/*
>  		 * The write-behind io is only attempted on drives marked as
>  		 * write-mostly, which means we could allocate write behind
> -		 * bio later.
> +		 * bio later. P2PDMA bios are excluded: write-behind copies
> +		 * the data with bio_copy_data(), a CPU copy that cannot be
> +		 * assumed safe or fast on P2PDMA (device BAR) pages.
>  		 */
> -		if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags))
> +		if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags) &&
> +		    !md_bio_is_p2pdma(bio))
>  			write_behind = true;
>  

[Severity: Critical]
This is a pre-existing issue, but by excluding P2PDMA bios from
write-behind, does this expose them to silent data corruption due to
missing serialization checks?

When a normal write to a sector uses write-behind, it can be acknowledged
to the caller once fast devices complete, leaving the write-behind bio
in-flight to the write-mostly device.

If a subsequent P2PDMA write is issued to the same sector, this patch
explicitly disables write-behind for it. Because of this,
r1_bio->behind_master_bio remains NULL, and execution falls into the else
branch further down in raid1_write_request():

drivers/md/raid1.c:raid1_write_request() {
    ...
		if (r1_bio->behind_master_bio) {
			mbio = bio_alloc_clone(rdev->bdev,
					       r1_bio->behind_master_bio,
					       GFP_NOIO, &mddev->bio_set);
			if (test_bit(CollisionCheck, &rdev->flags))
				wait_for_serialization(rdev, r1_bio);
                ...
		} else {
			mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO,
					       &mddev->bio_set);

			if (test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
				wait_for_serialization(rdev, r1_bio);
		}
    ...
}

Because the else branch skips the CollisionCheck test,
wait_for_serialization() is bypassed (unless MD_SERIALIZE_POLICY is globally
set). Can the block layer then reorder these concurrent requests, overwriting
the newer P2PDMA data with the older in-flight write-behind data?

>  		r1_bio->bios[i] = NULL;

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