Re: [PATCH] block: let a stacking device inherit its members' DMA alignment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] block: blk_set_stacking_limits() corrupts data in dm-ebs and dm-writecache
--

commit da5c2e4cde4a12a4f8b254dc27480cf9078b36d2
Author: Mike Snitzer <snitzer@kernel.org>

block: let a stacking device inherit its members' DMA alignment

This commit removes the default sector-size dma_alignment initialization in
blk_set_stacking_limits() so that stacking devices inherit the strictest
dma_alignment of their underlying member devices. When no member devices are
stacked, blk_validate_limits() still applies the default sector alignment.

> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index e469baa1f08b7..36fcbecdf793b 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -41,7 +41,14 @@ void blk_set_stacking_limits(struct queue_limits *lim)
>  	lim->physical_block_size = SECTOR_SIZE;
>  	lim->io_min = SECTOR_SIZE;
>  	lim->discard_granularity = SECTOR_SIZE;
> -	lim->dma_alignment = SECTOR_SIZE - 1;
> +	/*
> +	 * No dma_alignment of its own: blk_stack_limits() keeps the strictest
> +	 * of the devices stacked under it, and blk_validate_limits() gives the
> +	 * sector-size default to a device left with none.  A driver that looks
> +	 * at the data and needs it laid out by logical block says so in its
> +	 * own limits, as dm-crypt, dm-integrity, dm-verity and dm-log-writes
> +	 * do.
> +	 */

[Severity: Critical]
Could leaving lim->dma_alignment at 0 in blk_set_stacking_limits() cause data
corruption regressions in targets like dm-ebs and dm-writecache that inspect
bio data but do not set limits->dma_alignment in their io_hints callbacks?

When stacked over a device with sub-sector dma_alignment (such as an nvme
device with dma_alignment = 3), direct I/O using a 4-byte-aligned user buffer
that crosses a page boundary or uses sub-sector iovecs can produce bio_vec
segments whose lengths are not multiples of 512 bytes (for example,
bv_len = 4092 followed by bv_len = 4).

In drivers/md/dm-ebs-target.c, ebs_io_hints() does not set
limits->dma_alignment:

drivers/md/dm-ebs-target.c:ebs_io_hints() {
	struct ebs_c *ec = ti->private;

	limits->logical_block_size = to_bytes(ec->e_bs);
	limits->physical_block_size = to_bytes(ec->u_bs);
	limits->alignment_offset = limits->physical_block_size;
	limits->io_min = limits->logical_block_size;
}

When __ebs_rw_bio() iterates over the bio with bio_for_each_bvec() and calls
__ebs_rw_bvec(), __ebs_rw_bvec() computes the buffer offset and block number
from iter->bi_sector:

drivers/md/dm-ebs-target.c:__ebs_rw_bvec() {
	unsigned int bv_len = bv->bv_len;
	unsigned int buf_off = to_bytes(__block_mod(iter->bi_sector, ec->u_bs));
	sector_t block = __sector_to_block(ec, iter->bi_sector);

Because bio_for_each_bvec() advances iter via bio_advance_iter_single() in
include/linux/bio.h:

include/linux/bio.h:bio_advance_iter_single() {
	iter->bi_sector += bytes >> 9;

a segment with bv_len = 4092 advances iter.bi_sector by 4092 >> 9 = 7 sectors
(3584 bytes) instead of 4092 bytes, wouldn't the subsequent bio_vec read or
write at the wrong offset in the dm-bufio block?

Similarly, in drivers/md/dm-writecache.c, writecache_io_hints() only calls
dm_stack_bs_limits() and leaves limits->dma_alignment untouched:

drivers/md/dm-writecache.c:writecache_io_hints() {
	struct dm_writecache *wc = ti->private;

	dm_stack_bs_limits(limits, wc->block_size);
}

In persistent memory mode, writecache_map_read() and writecache_map_write()
process multi-block bios in a loop using bio->bi_iter.bi_sector to look up
and record cache entry sectors via write_original_sector_seq_count(), calling
bio_copy_block() for each block:

drivers/md/dm-writecache.c:bio_copy_block() {
	do {
		struct bio_vec bv = bio_iter_iovec(bio, bio->bi_iter);
		...
		size = bv.bv_len;
		if (unlikely(size > remaining_size))
			size = remaining_size;
		...
		data = (char *)data + size;
		remaining_size -= size;
		bio_advance(bio, size);
	} while (unlikely(remaining_size));
}

When a cache block spans multiple bio_vec segments with sub-sector lengths
(such as 4092 and 4 for a 4096-byte block), bio_advance() ->
bio_advance_iter() updates bio->bi_iter.bi_sector += size >> 9 on each step,
advancing bi_sector by 7 sectors instead of 8 and corrupting
bio->bi_iter.bi_sector for subsequent blocks in the bio.

In addition, when bio_copy_block() calls memcpy_flushcache_optimized() with a
non-64-byte-multiple size >= 768 (such as 4092), doesn't the loop return
early without copying the remaining size % 64 bytes?

drivers/md/dm-writecache.c:memcpy_flushcache_optimized() {
	if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT) &&
	    likely(boot_cpu_data.x86_clflush_size == 64) &&
	    likely(size >= 768)) {
		do {
			memcpy((void *)dest, (void *)source, 64);
			clflushopt((void *)dest);
			dest += 64;
			source += 64;
			size -= 64;
		} while (size >= 64);
		return;
	}
}

Finally, in solid-state cache mode, writecache_iterate_devices() only stacks
limits from wc->dev and not wc->ssd_dev:

drivers/md/dm-writecache.c:writecache_iterate_devices() {
	struct dm_writecache *wc = ti->private;

	return fn(ti, wc->dev, 0, ti->len, data);
}

If wc->dev has dma_alignment = 3 while wc->ssd_dev has dma_alignment = 511,
wouldn't dm-writecache advertise dma_alignment = 3 and remap 4-byte-aligned
bios to wc->ssd_dev->bdev, causing bio_split_io_at() to reject them?

>  	lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
>  
>  	/* Inherit limits from component devices */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/atpBVtPqGDWZ2AFo@kernel.org?part=1