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

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Oct 22, 2026 at 05:25:02PM +0000, sashiko-bot@kernel.org wrote:
> 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).

The code comment in the patch calls out the fact that
blk_validate_limits() will backfill dma_alignment if none is set (by
usual limits stacking or explicitly by DM targets).

But sure, there is potential for dragons if stacked block devices
like DM implicitly relied on block-core's coarse-grained default
dma_alignment.

Mikulas, would you be open to auditing the DM targets relative to
dma_alignment stacking after you're done with reviewing Keith's
related dm-crypt patchset?

Sorry for the extra work... but its worthwhile for DM to be more
precise with how it stacks dma_alignment

Thanks,
Mike

> 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