Thank you for the review! Regarding the first issue (memcmp ignores page-in-page offset), you are correct. This will be fixed in the v2 version of this patch, which adds offset_in_page(pos1)/offset_in_page(pos2) to the saddr/daddr arguments in the memcmp() call.
Regarding the second issue (bypassed length check when pfnp is NULL), this is a pre-existing behavior of dax_iomap_direct_access() shared by all callers that pass a NULL pfnp (e.g., dax_iomap_copy_around(), dax_unshare_iter()). In the dedupe path, the access range is guaranteed to be within the iomap mapping via len = min(len, min(smap->length, dmap->length)), and filesystem iomap mappings do not cross device boundaries. This is outside the scope of this patch and would be better addressed as a separate fix to dax_iomap_direct_access() itself. On Sat, Oct 10, 2026 at 3:35 PM <[email protected]> wrote: > > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) > to consider: > - [High] fsdax: dax_range_compare_iter() memcmp ignores page-in-page offset > - [High] fsdax: out-of-bounds access due to bypassed length check in > dax_iomap_direct_access() > -- > > commit 05244bba82891c8d959214ace5da854f182c9e20 > Author: Jianyun Gao <[email protected]> > > dax: fix wrong size passed to dax_iomap_direct_access() in > dax_range_compare_iter() > > dax_range_compare_iter() passes ALIGN(pos + len, PAGE_SIZE) as the size > argument to dax_iomap_direct_access(), but this is an absolute file > offset rather than a length. This causes the underlying > dax_direct_access() to request a massively inflated nr_pages via > PHYS_PFN(size), leading to unnecessarily large access ranges and > potential spurious -EHWPOISON from driver badblock scans. > > > diff --git a/fs/dax.c b/fs/dax.c > > index 08adeaed2baf1..fb12d0c0dcd49 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -2190,6 +2190,7 @@ static int dax_range_compare_iter(struct iomap_iter > > *it_src, > > const struct iomap *dmap = &it_dest->iomap; > > loff_t pos1 = it_src->pos, pos2 = it_dest->pos; > > void *saddr, *daddr; > > + size_t size1, size2; > > int id, ret; > > > > len = min(len, min(smap->length, dmap->length)); > > @@ -2205,13 +2206,13 @@ static int dax_range_compare_iter(struct iomap_iter > > *it_src, > > } > > > > id = dax_read_lock(); > > - ret = dax_iomap_direct_access(smap, pos1, ALIGN(pos1 + len, > > PAGE_SIZE), > > - &saddr, NULL); > > + size1 = ALIGN(len + (pos1 & ~PAGE_MASK), PAGE_SIZE); > > + ret = dax_iomap_direct_access(smap, pos1, size1, &saddr, NULL); > > [Severity: High] > Does this code risk an out-of-bounds access in the subsequent memcmp() if the > underlying physical DAX memory mapped is shorter than requested? > > Looking at dax_iomap_direct_access(), it bypasses the length validation when > the pfnp argument is NULL: > > fs/dax.c:dax_iomap_direct_access() { > ... > if (!pfnp) > goto out_check_addr; > rc = -EINVAL; > if (PFN_PHYS(length) < size) > goto out; > ... > } > > If the iomap extent crosses a device mapper target boundary, such as on > dm-linear, dm_dax_direct_access() truncates the mapping length and returns > fewer pages than requested. > > Because dax_iomap_direct_access() is called here with a NULL pfnp, it skips > the length check and returns success even for short mappings. > > Could this regression cause the subsequent memcmp() to read beyond the > physically contiguous mapped memory? > > > if (ret < 0) > > goto out_unlock; > > > > - ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, > > PAGE_SIZE), > > - &daddr, NULL); > > + size2 = ALIGN(len + (pos2 & ~PAGE_MASK), PAGE_SIZE); > > + ret = dax_iomap_direct_access(dmap, pos2, size2, &daddr, NULL); > > if (ret < 0) > > goto out_unlock; > > > > [Severity: High] > Is it possible that the memcmp() that follows compares the wrong file data > because it ignores the page-in-page offset? > > The dax_iomap_direct_access() function translates a file offset to a kernel > virtual address, but the returned pointers saddr and daddr are strictly > page-aligned, ignoring any page-in-page offset (pos & ~PAGE_MASK). > > When filesystem block sizes are smaller than the architecture's page size, > deduplication requests can be block-aligned but not page-aligned. > > If memcmp() is called directly on saddr and daddr without adding the > pos1 & ~PAGE_MASK and pos2 & ~PAGE_MASK offsets: > > fs/dax.c:dax_range_compare_iter() { > ... > *same = !memcmp(saddr, daddr, len); > ... > } > > Will this regression compare memory starting from the page boundary instead of > the actual file offset? Could this lead to FIDEDUPERANGE incorrectly > deduplicating non-identical blocks and causing data corruption? > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=1
