Re: [PATCH] mm/truncate: unmap DAX entries before invalidating them

[Alistair Popple <apopple@nvidia.com>]

On 2026-11-07 at 08:43 +1100, Zi Yan <ziy@nvidia.com> wrote...
> invalidate_inode_pages2_range() invalidates the clean DAX entries in the
> range through dax_invalidate_mapping_entry_sync() and only unmaps the range
> afterwards. Dropping the last association of an entry releases its folio
> by dissolving the folio into order-0 ones. If the entry is still mapped,
> 1. dax_folio_put() warns;
> 2. if the folio order is PMD, folio_remove_rmap_pmd() during the unmap
>    afterwards operates on an order-0 folio (the original head page) and
>    corrupts the original first tail page metadata.
> 
> This is reachable from userspace with a copy-on-write write to a mapped
> reflinked file on XFS with DAX:
> 
>   WARNING: fs/dax.c:452 at dax_disassociate_entry.isra.0+0x147/0x1c0
>   Call Trace:
>    __dax_invalidate_entry+0x106/0x320
>    invalidate_inode_pages2_range+0x3ce/0x830
>    dax_iomap_rw+0x421/0xcc0
>    xfs_file_dax_write+0x252/0x9e0
>    vfs_write+0x53a/0xe70
>    __x64_sys_pwrite64+0x195/0x250
>    ...
>   WARNING: ./include/linux/rmap.h:351 at folio_remove_rmap_pmd+0x8b2/0xaf0
>   Call Trace:
>    split_huge_pmd_locked+0x751/0x2350
>    __split_huge_pmd+0x1bb/0x310
>    ...
>    unmap_mapping_pages+0x133/0x150
>    invalidate_inode_pages2_range+0x700/0x830
>    dax_iomap_rw+0x421/0xcc0
>    ...
> 
> To fix it, unmap DAX ranges before invalidating them, as the page cache
> path does for mapped folios. In __dax_invalidate_entry(), when
> invalidating, skip an entry if the code is dropping its last association
> but its folio still has outstanding references. -EBUSY is returned for the
> skipped entries. The unmap before invalidation works because any remap of
> the folio between the unmap and invalidation will bump the folio refcount.

A quick check of callers of invalidate_inode_pages2_range() either ignore
the return code or produce a WARN_ON if the invalidation fails. For example
xfs_fs_commit_blocks(). I don't think anything makes that inherently safe
against COW for example.

That's pre-existing of course - but this patch does increase the likelihood of
error. Overall though I think this patch is still an improvement, and based on
code comments my AI agent concluded that these kind of issues are somewhat known
anyway.

> Fixes: 38607c62b34b ("fs/dax: properly refcount fs dax pages")

I suspect this issue actually preexists my changes here. Prior the page
meta-data was still getting modified on disassociation even if the folio was
referenced. In some ways proper refcounting just made it easier to catch these
kind of problems, which was partly the point of doing it :-)

But I think it makes sense to use this as the "Fixes" tag - backporting this to
anything prior would be difficult and a rewrite of this fix anyway.

> Cc: stable@vger.kernel.org
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> I found this while auditing the paths that map pages into user page
> tables. invalidate_inode_pages2_range() releases the folios of DAX
> entries that are still mapped. For a PMD-mapped folio this corrupts what
> used to be its first tail page, and a copy-on-write write to a mapped
> reflinked file on XFS with DAX is enough to hit it.
> 
> In QEMU with emulated NVDIMMs and XFS on an fsdax namespace:
> 
> - Copy-on-write with the script below, PTE- and PMD-mapped, with only a
>   mapped and with a and b mapped. Before this patch, the WARNs in the
>   commit message and a corrupted first tail page in the PMD case. With this
>   patch: the mapping of a sees the new data, b keeps the old data, and
>   nothing warns, except for item 4 below.
> 
>   mkfs.xfs -f -m reflink=1 -d su=2m,sw=1 /dev/pmem1
>   mount -o dax=always /dev/pmem1 /mnt && cd /mnt
>   xfs_io -f -c "extsize 2m" -c "pwrite -q -S 0x5a 0 4m" -c fsync a
>   xfs_io -f -c "reflink a 0 0 4m" b
>   (echo "mmap -r 0 4m"; echo "madvise -h 0 4m"; echo "mread 0 4m";
>    sleep 4; echo "mread -v 0 16") | xfs_io a &
>   sleep 2; xfs_io -c "pwrite -q -S 0xa5 0 8k" -c fsync a; wait

Ideally I think this should be added to the xfstest suite - that was extremely
useful during some of the refcounting reworks and found all sorts of interesting
corner cases.

> Open issues that this patch does not address. They are races or
> already exist without it.
> 
> 1. A refault, or a short-term or speculative reference, between the
>    early unmap and the invalidation keeps a last association in place,
>    and invalidate_inode_pages2_range() returns -EBUSY.
>    dax_iomap_iter(), dax_unshare_iter() and dax_zero_iter() ignore that
>    and write the new extent anyway, which leaves a stale entry. Without
>    this patch the same race tears down the referenced folio. Should
>    these paths retry, or fail?
> 
> 2. XFS device pre-removal ignores the result as well, so an entry kept
>    because of a short-term reference is not revisited before the
>    memmap goes away.
> 
> 3. folio->share is updated under each file's i_pages lock, so the
>    shared-count check here, like dax_folio_put() itself, can race with
>    another file dropping its share.
> 
> 4. With reflink, after a copy-on-write write, a PTE fault can associate
>    an order-0 entry with a shared folio that another file still maps
>    with a PMD. That trips WARN_ON_ONCE(dax_entry_order(entry) !=
>    folio_order(folio)) in dax_associate_entry(), with and without this
>    patch.
> 
> Based on an LLM's analysis, 1 and 2 can be fixed by holding the invalidate
> lock and breaking DAX layouts in the callers; 3 needs a per-folio lock
> for ->share; 4 needs a way to share a folio at different granularities,
> or no PMD mappings of shared extents as a simple fix.
> 
> LLMs were used to analyze the issue, write the patch, and review the code.
> ---
>  fs/dax.c      | 28 ++++++++++++++++++++++++++--
>  mm/truncate.c | 16 +++++++++++-----
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 94ba41273347c..e448552d8080d 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -527,6 +527,24 @@ static struct page *dax_busy_page(void *entry)
>  		return NULL;
>  }
>  
> +/*
> + * Determine if an entry can be disassociated. It cannot be if it is its folio's
> + * last association and the folio still has a reference.
> + */
> +static bool dax_can_disassociate(void *entry)
> +{
> +	struct folio *folio;
> +
> +	if (dax_is_zero_entry(entry) || dax_is_empty_entry(entry))
> +		return true;
> +
> +	folio = dax_to_folio(entry);
> +	if (dax_folio_is_shared(folio) && folio->share > 1)
> +		return true;
> +
> +	return !folio_ref_count(folio);
> +}
> +
>  /**
>   * dax_lock_folio - Lock the DAX entry corresponding to a folio
>   * @folio: The folio whose entry we want to lock
> @@ -853,9 +871,14 @@ static int __dax_invalidate_entry(struct address_space *mapping,
>  	entry = get_next_unlocked_entry(&xas, 0);
>  	if (!entry || WARN_ON_ONCE(!xa_is_value(entry)))
>  		goto out;
> +	/*
> +	 * Removing the last association of a folio releases the folio, so make
> +	 * sure the folio has no reference.
> +	 */
>  	if (!trunc &&
>  	    (xas_get_mark(&xas, PAGECACHE_TAG_DIRTY) ||
> -	     xas_get_mark(&xas, PAGECACHE_TAG_TOWRITE)))
> +	     xas_get_mark(&xas, PAGECACHE_TAG_TOWRITE) ||
> +	     !dax_can_disassociate(entry)))
>  		goto out;
>  	dax_disassociate_entry(entry, mapping, trunc);
>  	xas_store(&xas, NULL);
> @@ -1017,7 +1040,8 @@ void dax_break_layout_final(struct inode *inode)
>  EXPORT_SYMBOL_GPL(dax_break_layout_final);
>  
>  /*
> - * Invalidate DAX entry if it is clean.
> + * Invalidate DAX entry if it is clean and dropping it would not release a folio
> + * that is still in use.
>   */
>  int dax_invalidate_mapping_entry_sync(struct address_space *mapping,
>  				      pgoff_t index)
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 9e9bf029635cf..b03cd99c4bba8 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -732,6 +732,15 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
>  	if (mapping_empty(mapping))
>  		return 0;
>  
> +	/*
> +	 * Invalidating a DAX entry can release its folio, so unmap DAX entries
> +	 * before invalidating them. dax_invalidate_mapping_entry_sync() skips
> +	 * the last association of a folio that still has references, e.g.
> +	 * because it was faulted in again in the meantime.
> +	 */
> +	if (dax_mapping(mapping))
> +		unmap_mapping_pages(mapping, start, end - start + 1, false);
> +
>  	folio_batch_init(&fbatch);
>  	index = start;
>  	while (find_get_entries(mapping, &index, end, &fbatch, indices)) {
> @@ -787,11 +796,8 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
>  		cond_resched();
>  	}
>  	/*
> -	 * For DAX we invalidate page tables after invalidating page cache.  We
> -	 * could invalidate page tables while invalidating each entry however
> -	 * that would be expensive. And doing range unmapping before doesn't
> -	 * work as we have no cheap way to find whether page cache entry didn't
> -	 * get remapped later.
> +	 * For DAX, also zap the mappings that were faulted in while we were
> +	 * invalidating.
>  	 */
>  	if (dax_mapping(mapping)) {
>  		unmap_mapping_pages(mapping, start, end - start + 1, false);
> 
> ---
> base-commit: 63919467fb5239ab824de6a7a128b882055b24af
> change-id: 20261103-fix-dax-invalidate-mapped-d0c2296f5543
> 
> Best regards,
> --  
> Yan, Zi
>