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 <[email protected]> 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/[email protected]?part=1
