Re: [PATCH v2 5/7] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones

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

New issues:
- [High] The newly added `md_bio_is_p2pdma(wbio)` check evaluates a cloned bio after its operation type has been forcibly overwritten to `REQ_OP_WRITE`, defeating the `bio_has_data()` guard and leading to a kernel panic for data-less bios.

Pre-existing issues:
- [High] Pre-existing data integrity bug: `narrow_write_error()` unconditionally overwrites `bi_opf` to `REQ_OP_WRITE`, silently stripping essential flags like `REQ_FUA` and `REQ_PREFLUSH`.
- [High] Pre-existing block layer crash: `narrow_write_error()` blindly overwrites data-less operations with `REQ_OP_WRITE`, submitting invalid bios to underlying block drivers.
--

--- Patch [5]: [PATCH v2 5/7] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones ---
commit 21dea61208137a1862fe54775b5bbeea8b5691ab
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones

This commit fixes an issue where narrow_write_error drops the REQ_NOMERGE
flag on P2PDMA bios during a write error retry. It adds a check to restore
the flag for P2PDMA bios, preventing invalid merges across pgmaps.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 914fb86452c03..f562b6bd438b9 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2573,6 +2573,9 @@ static void narrow_write_error(struct r1bio *r1_bio, int i)
>  		}
>  
>  		wbio->bi_opf = REQ_OP_WRITE;

[Severity: High]
This isn't a bug introduced by this patch, but does the unconditional
assignment to REQ_OP_WRITE in narrow_write_error() strip essential flags
like REQ_FUA and REQ_PREFLUSH?

If an I/O error occurs on a write that carries these flags, dropping them
could silently break flush guarantees and cause data corruption after a
power failure. Could we preserve the existing flags via a mask instead?

[Severity: High]
This is also a pre-existing issue, but does overwriting data-less
operations with REQ_OP_WRITE in narrow_write_error() submit invalid bios
to the block drivers?

When operations like REQ_OP_WRITE_ZEROES fail, they are transformed into a
standard write with a non-zero bi_size but no initialized data vectors.
Attempting to map this payload for DMA via bio_for_each_segment() might
dereference uninitialized memory and cause a crash.

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;

[Severity: High]
Does checking md_bio_is_p2pdma() immediately after forcing the operation
type to REQ_OP_WRITE defeat the bio_has_data() guard?

Because the operation is now a standard write, bio_has_data() will return
true even for originally data-less operations. This would cause the function
to evaluate the uninitialized page pointer in the bio vector, potentially
leading to a kernel panic. Should the check evaluate the original bio
instead of the modified clone?

>  		wbio->bi_iter.bi_sector = r1_bio->sector;
>  		wbio->bi_iter.bi_size = r1_bio->sectors << 9;

[ ... ]

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..f7ef903a3d4ec 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2831,6 +2831,9 @@ static void narrow_write_error(struct r10bio *r10_bio, int i)
>  		wbio->bi_iter.bi_sector = wsector +
>  				   choose_data_offset(r10_bio, rdev);
>  		wbio->bi_opf = REQ_OP_WRITE;

[Severity: High]
This isn't a bug introduced by this patch, but this overwrite has the same
issue as in raid1.c. Does this unconditional assignment strip operation flags
like REQ_FUA and REQ_PREFLUSH, leading to potential data integrity issues?

[Severity: High]
This is also a pre-existing issue, but does this similarly submit invalid
bios to the block layer by changing failed data-less operations into standard
writes without initialized data pages?

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;

[Severity: High]
Similar to the raid1.c code, does calling md_bio_is_p2pdma() right after
the operation code is forced to REQ_OP_WRITE neutralize the bio_has_data()
guard? Evaluating the uninitialized memory for data-less operations could
cause a kernel panic here as well.

>  
>  		if (submit_bio_wait(wbio) &&
>  		    !rdev_set_badblocks(rdev, wsector, sectors, 0)) {

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