Re: [PATCH] fs/dax: check for empty/zero entries before calling pfn_to_page()

[Alistair Popple <apopple@nvidia.com>]

On 2026-05-08 at 19:15 +1000, "David Hildenbrand (Arm)" <david@kernel.org> wrote...
> On 5/2/26 01:39, Souvik Banerjee wrote:
> > Commit 98c183a4fccf ("fs/dax: don't disassociate zero page entries")
> > added zero/empty-entry early returns to dax_associate_entry() and
> > dax_disassociate_entry(), but placed them *after* the
> > `struct folio *folio = dax_to_folio(entry);` line.  dax_to_folio()
> > expands to page_folio(pfn_to_page(dax_to_pfn(entry))), and page_folio()
> > performs READ_ONCE(page->compound_head) -- a real dereference of the
> > struct page pointer derived from a bogus PFN extracted from the
> > empty/zero XA value.
> > 
> > On systems where vmemmap covers all of RAM that dereference reads
> > garbage and is harmless: the early return then discards the result.
> > On virtio-pmem with altmap (vmemmap stored inside the device), only
> > the real device PFN range is mapped, so the dereference triggers a
> > kernel paging fault from the truncate / invalidate path and from the
> > PMD-downgrade branch of dax_iomap_pte_fault when an entry is being
> > freed:
> > 
> >   Unable to handle kernel paging request at
> >   virtual address ffff_fdff_bf00_0008 (vmemmap region)
> >   Call trace:
> >    dax_disassociate_entry.isra.0+0x20/0x50
> >    dax_iomap_pte_fault
> >    dax_iomap_fault
> >    erofs_dax_fault
> > 
> > Close the residual gap by moving the dax_to_folio() call after the
> > zero/empty guard in dax_disassociate_entry().  Apply the same
> > treatment to dax_busy_page(), which has the identical pattern but
> > was not touched by the prior fix.
> > 
> > Fixes: 98c183a4fccf ("fs/dax: don't disassociate zero page entries")
> > Fixes: 38607c62b34b ("fs/dax: properly refcount fs dax pages")
> > Cc: stable@vger.kernel.org # v6.15+
> > Cc: Alistair Popple <apopple@nvidia.com>

Thanks for fixing this.

> > Signed-off-by: Souvik Banerjee <souvik@amlalabs.com>
> > ---
> >  fs/dax.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 6d175cd47a99..6878473265bb 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -505,21 +505,23 @@ static void dax_associate_entry(void *entry, struct address_space *mapping,
> >  static void dax_disassociate_entry(void *entry, struct address_space *mapping,
> >  				bool trunc)
> >  {
> > -	struct folio *folio = dax_to_folio(entry);
> > +	struct folio *folio;
> >  
> >  	if (dax_is_zero_entry(entry) || dax_is_empty_entry(entry))
> >  		return;
> >  
> > +	folio = dax_to_folio(entry);
> >  	dax_folio_put(folio);
> >  }
> >  
> >  static struct page *dax_busy_page(void *entry)
> >  {
> > -	struct folio *folio = dax_to_folio(entry);
> > +	struct folio *folio;
> >  
> >  	if (dax_is_zero_entry(entry) || dax_is_empty_entry(entry))
> >  		return NULL;
> >  
> > +	folio = dax_to_folio(entry);
> >  	if (folio_ref_count(folio) - folio_mapcount(folio))
> >  		return &folio->page;
> >  	else
> 
> Makes perfect sense to me.
> 
> 
> What about the usage in dax_associate_entry()?

Pretty sure the issue exists there as well given this code path implies we could
pass zero/empty entries there as well:

	if (shared || dax_is_zero_entry(entry) || dax_is_empty_entry(entry)) {
		void *old;

		dax_disassociate_entry(entry, mapping, false);
		dax_associate_entry(new_entry, mapping, vmf->vma,
					vmf->address, shared);

 - Alistair

> -- 
> Cheers,
> 
> David