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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] mm/truncate: unmap full DAX_PMD range in
  invalidate_inode_pages2_range()
- [High] fs/dax: premature disassociation of shared folios in
  dax_can_disassociate()
- [High] fs/dax: stale i_pages entry when __dax_invalidate_entry() skips
  busy folio
- [Medium] dax: cross-file TOCTOU and data race on folio->share in
  dax_can_disassociate()
--

commit cd725b6545b68d39f6ee84c70fd91581edad91be
Author: Zi Yan <ziy@nvidia.com>

mm/truncate: unmap DAX entries before invalidating them

This commit unmaps DAX ranges before invalidating entries in
invalidate_inode_pages2_range() and skips disassociating clean DAX entries
whose last association still has elevated folio references.

> 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);
> +}

[Severity: High]
Can returning true whenever folio->share > 1 allow a shared PMD folio to be
dissolved while it is still PMD-mapped?

During a CoW write on file A, xfs_file_dax_write() holds XFS_IOLOCK_EXCL
without XFS_MMAPLOCK_EXCL, so a read fault on file A can refault the shared
PMD between the initial unmap_mapping_pages() and __dax_invalidate_entry():

CPU0 (CoW write on file A)        CPU1 (fault on A / truncate on B)
invalidate_inode_pages2_range(A)
  unmap_mapping_pages(A)
                                  xfs_dax_read_fault(A)
                                    dax_fault_iter()
                                      vmf_insert_folio_pmd()
                                      (refcount == 1, mapcount == 1)
  __dax_invalidate_entry(A)
    dax_can_disassociate()
      folio->share == 2 -> true
    dax_disassociate_entry()
      --folio->share (now 1)
                                  dax_break_layout(B)
                                    unmap_mapping_pages(B)
                                    dax_busy_page() -> NULL
                                      (refcount - mapcount == 0)
                                    dax_delete_mapping_entry(B)
                                      --folio->share (now 0)
                                      dax_folio_reset_order()
  unmap_mapping_pages(A)
    folio_remove_rmap_pmd()  <-- corrupts dissolved tail page

In addition, if file A holds a GUP pin on the shared folio,
dax_can_disassociate() removes the entry from file A's i_pages while
folio->share > 1, so a later dax_break_layout() on file A will no longer find
the entry in i_pages or wait for the GUP pin.

[Severity: Medium]
Can concurrent invalidations on two files sharing a folio race on
folio->share in dax_can_disassociate()?

__dax_invalidate_entry() only holds the current mapping's i_pages lock:

CPU0 (file A)                     CPU1 (file B)
__dax_invalidate_entry(A)         __dax_invalidate_entry(B)
  xas_lock_irq(&A->i_pages)         xas_lock_irq(&B->i_pages)
  dax_can_disassociate()            dax_can_disassociate()
    folio->share == 2 -> true         folio->share == 2 -> true
  dax_folio_put()                   dax_folio_put()
    --folio->share                    --folio->share (reaches 0)
                                      dax_folio_reset_order()  <-- folio busy

Both callers bypass the folio_ref_count(folio) check when folio->share == 2
and then non-atomically decrement folio->share in dax_folio_put(), which can
either corrupt folio->share (a data race) or dissolve a referenced compound
folio when the second decrement reaches 0.

> @@ -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);

[Severity: High]
What happens when dax_can_disassociate() returns false during a CoW write and
leaves the old DAX entry in mapping->i_pages?

If the folio has an elevated refcount (for example from a short-term GUP pin
or a racing fault), __dax_invalidate_entry() skips the entry and
invalidate_inode_pages2_range() returns -EBUSY.  dax_iomap_iter(),
dax_unshare_iter() and dax_zero_iter() ignore that return value and complete
the CoW remap to a new unshared extent while the old entry remains in
mapping->i_pages.

On a subsequent fault to the now-unshared extent (shared == false),
dax_insert_entry() sees a non-empty entry and neither replaces it nor calls
dax_associate_entry() on the new PFN:

fs/dax.c:dax_insert_entry() {
    ...
	if (shared || dax_is_zero_entry(entry) || dax_is_empty_entry(entry)) {
		struct folio *folio = pfn_folio(pfn);
    ...
}

dax_fault_iter() then takes a reference on the old folio from *entry while
passing the uninitialized, zero-refcount new PFN to vmf_insert_folio_pmd() or
vmf_insert_page_mkwrite(), which fails with VM_FAULT_SIGBUS.  Meanwhile, the
stale entry left in the first file's i_pages still points to the physical
folio now owned exclusively by the other reflinked file, so truncating or
evicting the first file later calls dax_folio_put() ->
dax_folio_reset_order() on the second file's live PMD folio.

[ ... ]

> 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;

[Severity: High]
Does unmapping only [start, end] leave PTE mappings in place when [start, end]
partially overlaps an order-9 DAX_PMD entry in mapping->i_pages?

grab_mapping_entry() and dax_insert_entry() keep an existing storage-backed
DAX_PMD entry in mapping->i_pages when servicing PTE faults (such as in a
smaller or unaligned VMA).  When a sub-PMD write calls
invalidate_inode_pages2_range(mapping, start, end), unmap_mapping_pages()
only unmaps [start, end], whereas dax_invalidate_mapping_entry_sync() ->
__dax_invalidate_entry() disassociates and removes the entire 2MB DAX_PMD
entry covering [index & ~PG_PMD_COLOUR, index | PG_PMD_COLOUR].

If the PMD folio is not shared (or this is its last share), the un-zapped PTEs
outside [start, end] keep folio_ref_count(folio) > 0, so
dax_can_disassociate() deterministically fails with -EBUSY and leaves the
stale DAX_PMD entry in i_pages after CoW remapping.  If folio->share > 1,
dax_can_disassociate() succeeds and drops this file's share while those PTEs
outside [start, end] remain mapped, so when the other file drops the last
share, dax_folio_put() dissolves the order-9 folio while its tail pages are
still mapped via PTEs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-fix-dax-invalidate-mapped-v1-1-292ce5958d67@nvidia.com?part=1