Regarding the first issue (`dax_dedupe_file_range_compare()` drops error and returns false dedup success), this is already addressed by a separate patch that changes `return ret` to `return status`: https://lore.kernel.org/lkml/[email protected]/
Regarding the second issue (memcmp on IOMAP_UNWRITTEN exposes uninitialized PMEM data), this is a pre-existing issue unrelated to the current patch. The current patch only fixes the size and address offset bugs and does not change the handling of IOMAP_UNWRITTEN. Regarding the third issue (CPU soft lockup due to unbounded memcmp), this is also a pre-existing concern. `dax_range_compare_iter()` operates within an iomap iteration that bounds each iteration's length to a single iomap mapping via `len = min(len, min(smap->length, dmap->length))`, so memcmp typically does not run across the entire 1GB dedupe request. On Sun, Oct 11, 2026 at 3:09 PM <[email protected]> wrote: > > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) > to consider: > - [High] fs/dax: memcmp on IOMAP_UNWRITTEN exposes uninitialized PMEM data > via dedupe > - [High] fsdax: CPU soft lockup due to unbounded memcmp in dedupe > - [High] fs/dax: dax_dedupe_file_range_compare() drops error and returns > false dedup success > -- > > commit 035a3cbae242fa7d9d78dcd078b21ab3eeaecc67 > Author: Jianyun Gao <[email protected]> > > dax: fix dax_range_compare_iter() to use correct size and address offset > > This patch fixes two bugs in dax_range_compare_iter() when calling > dax_iomap_direct_access(). It corrects the size argument to use a length > rather than an absolute file offset, and it adds the page-in-page offset to > the source and destination addresses before comparing them with memcmp(). > > > diff --git a/fs/dax.c b/fs/dax.c > > index 08adeaed2baf1..7bee4314f905b 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,17 +2206,18 @@ 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 + offset_in_page(pos1), PAGE_SIZE); > > + ret = dax_iomap_direct_access(smap, pos1, size1, &saddr, NULL); > > if (ret < 0) > > goto out_unlock; > > [Severity: High] > If dax_iomap_direct_access() fails here (e.g., from an I/O error or a size > issue) and returns a negative error code, does the upstream caller > dax_dedupe_file_range_compare() incorrectly mask the failure and report false > success? > > When this function returns a negative error code, > dax_dedupe_file_range_compare() handles it like this: > > fs/dax.c:dax_dedupe_file_range_compare() { > ... > status = dax_range_compare_iter(&src_iter, &dst_iter, > min(src_iter.len, dst_iter.len), same); > if (status < 0) > return ret; > ... > } > > Because ret holds a positive bytes count from the loop's earlier iomap_iter() > call, a positive value is propagated back to vfs_dedupe_file_range(). > > The VFS intercepts the positive return as a success flag, bypasses block > remapping, and reports a successful deduplication result to userspace even > though the remapping aborted early. > > > > > - ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, > > PAGE_SIZE), > > - &daddr, NULL); > > + size2 = ALIGN(len + offset_in_page(pos2), PAGE_SIZE); > > + ret = dax_iomap_direct_access(dmap, pos2, size2, &daddr, NULL); > > if (ret < 0) > > goto out_unlock; > > > > - *same = !memcmp(saddr, daddr, len); > > + *same = !memcmp(saddr + offset_in_page(pos1), > > + daddr + offset_in_page(pos2), len); > > [Severity: High] > Does this code expose uninitialized PMEM data if the extent is > IOMAP_UNWRITTEN? > > While dax_range_compare_iter() explicitly checks for and skips IOMAP_HOLE > earlier in the function, it fails to check for IOMAP_UNWRITTEN. > > As a result, the physical PMEM blocks for unwritten extents (e.g., from > fallocate) are mapped by dax_iomap_direct_access() without zero-filling > them. > > This memcmp() then directly compares this uninitialized data against the > target extent. > > Since early-exit from memcmp() is measurable, could this be used as a > timing side-channel to extract previous persistent memory contents, as well > as causing functional deduplication failures when comparing logically-zero > unwritten extents? > > [Severity: High] > Can this memcmp() cause a CPU soft lockup and RCU stall? > > When called via the FIDEDUPERANGE ioctl, vfs_dedupe_file_range() can cap the > deduplication length to 1GB. > > Because DAX/PMEM mappings can be contiguous for gigabytes, this length could > be passed directly to a single synchronous memcmp() call here without any > cond_resched() yielding points. > > > if (!*same) > > len = 0; > > dax_read_unlock(id); > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=1
