Re: [PATCH v5 06/10] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones

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

Pre-existing issues:
- [High] narrow_write_error() dangerously converts failed dataless operations into normal writes (REQ_OP_WRITE), causing crashes during trimming or block layer submission.
- [High] narrow_write_error() resets bi_opf to a bare REQ_OP_WRITE, silently dropping critical data integrity flags like REQ_FUA.
--

--- Patch [6]: [PATCH v5 06/10] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones ---
commit 63dc9314df6c2490722e42dfb137e9e52bd9544f
Author: Mykola Marzhan <[email protected]>

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

narrow_write_error() re-issues a failed write in badblock-granularity chunks,
cloning from the master bio and resetting bi_opf to a bare REQ_OP_WRITE. This
drops REQ_NOMERGE for P2PDMA bios, so the patch restores the flag on P2PDMA
retry clones.

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

[Severity: High]
This is a pre-existing issue, but does forcing wbio->bi_opf to REQ_OP_WRITE
cause crashes for dataless operations?

If an underlying device fails an operation like REQ_OP_SECURE_ERASE or
REQ_OP_WRITE_ZEROES, forcing it to REQ_OP_WRITE means it is no longer
recognized as dataless.

Would bio_trim() then attempt to advance the bio iterator and dereference
a NULL bi_io_vec array? Even if it doesn't span multiple badblocks and
bypasses the trim crash, would it be submitted to the lower-level driver as
a malformed payloadless write?

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;
>  		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 5b63175fb3826..428bddddfe71d 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2823,6 +2823,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 does resetting bi_opf to a
bare REQ_OP_WRITE silently drop critical data integrity flags?

By overwriting the opcode rather than modifying it, it appears flags like
REQ_FUA and REQ_SYNC that were present on the master bio would be lost on the
retry clone, potentially leading to data loss if a power failure occurs.

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;
>  
>  		if (submit_bio_wait(wbio) &&
>  		    !rdev_set_badblocks(rdev, wsector, sectors, 0)) {

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