Re: [PATCH] mm/x86/pat: Only untrack the pfn range if unmap region

[Peter Xu]

On Fri, Jul 19, 2024 at 10:18:12PM -0400, Liam R. Howlett wrote:
> * Peter Xu <peterx@xxxxxxxxxx> [240712 10:43]:
> > This patch is one patch of an old series [1] that got reposted standalone
> > here, with the hope to fix some reported untrack_pfn() issues reported
> > recently [2,3], where there used to be other fix [4] but unfortunately
> > which looks like to cause other issues.  The hope is this patch can fix it
> > the right way.
> > 
> > X86 uses pfn tracking to do pfnmaps.  AFAICT, the tracking should normally
> > start at mmap() of device drivers, then untracked when munmap().  However
> > in the current code the untrack is done in unmap_single_vma().  This might
> > be problematic.
> > 
> > For example, unmap_single_vma() can be used nowadays even for zapping a
> > single page rather than the whole vmas.  It's very confusing to do whole
> > vma untracking in this function even if a caller would like to zap one
> > page.  It could simply be wrong.
> > 
> > Such issue won't be exposed by things like MADV_DONTNEED won't ever work
> > for pfnmaps and it'll fail the madvise() already before reaching here.
> > However looks like it can be triggered like what was reported where invoked
> > from an unmap request from a file vma.
> > 
> > There's also work [5] on VFIO (merged now) to allow tearing down MMIO
> > pgtables before an munmap(), in which case we may not want to untrack the
> > pfns if we're only tearing down the pgtables.  IOW, we may want to keep the
> > pfn tracking information as those pfn mappings can be restored later with
> > the same vma object.  Currently it's not an immediate problem for VFIO, as
> > VFIO uses UC- by default, but it looks like there's plan to extend that in
> > the near future.
> > 
> > IIUC, this was overlooked when zap_page_range_single() was introduced,
> > while in the past it was only used in the munmap() path which wants to
> > always unmap the region completely.  E.g., commit f5cc4eef9987 ("VM: make
> > zap_page_range() callers that act on a single VMA use separate helper") is
> > the initial commit that introduced unmap_single_vma(), in which the chunk
> > of untrack_pfn() was moved over from unmap_vmas().
> > 
> > Recover that behavior to untrack pfnmap only when unmap regions.
> > 
> > [1] https://lore.kernel.org/r/20240523223745.395337-1-peterx@xxxxxxxxxx
> > [2] https://groups.google.com/g/syzkaller-bugs/c/FeQZvSbqWbQ/m/tHFmoZthAAAJ
> > [3] https://lore.kernel.org/r/20240712131931.20207-1-00107082@xxxxxxx
> > [4] https://lore.kernel.org/all/20240710-bug12-v1-1-0e5440f9b8d3@xxxxxxxxx/
> > [5] https://lore.kernel.org/r/20240523195629.218043-1-alex.williamson@xxxxxxxxxx
> > 
> > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Cc: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxxxx>
> > Cc: Kirill A. Shutemov <kirill@xxxxxxxxxxxxx>
> > Cc: x86@xxxxxxxxxx
> > Cc: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Cc: Pei Li <peili.dev@xxxxxxxxx>
> > Cc: David Hildenbrand <david@xxxxxxxxxx>
> > Cc: David Wang <00107082@xxxxxxx>
> > Cc: Bert Karwatzki <spasswolf@xxxxxx>
> > Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> > 
> > NOTE: I massaged the commit message comparing to the rfc post [1], the
> > patch itself is untouched.  Also removed rfc tag, and added more people
> > into the loop. Please kindly help test this patch if you have a reproducer,
> > as I can't reproduce it myself even with the syzbot reproducer on top of
> > mm-unstable.  Instead of further check on the reproducer, I decided to send
> > this out first as we have a bunch of reproducers on the list now..
> > ---
> >  mm/memory.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 4bcd79619574..f57cc304b318 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1827,9 +1827,6 @@ static void unmap_single_vma(struct mmu_gather *tlb,
> >  	if (vma->vm_file)
> >  		uprobe_munmap(vma, start, end);
> >  
> > -	if (unlikely(vma->vm_flags & VM_PFNMAP))
> > -		untrack_pfn(vma, 0, 0, mm_wr_locked);
> > -
> >  	if (start != end) {
> >  		if (unlikely(is_vm_hugetlb_page(vma))) {
> >  			/*
> > @@ -1894,6 +1891,8 @@ void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> >  		unsigned long start = start_addr;
> >  		unsigned long end = end_addr;
> >  		hugetlb_zap_begin(vma, &start, &end);
> > +		if (unlikely(vma->vm_flags & VM_PFNMAP))
> > +			untrack_pfn(vma, 0, 0, mm_wr_locked);
> >  		unmap_single_vma(tlb, vma, start, end, &details,
> >  				 mm_wr_locked);
> >  		hugetlb_zap_end(vma, &details);
> > -- 
> > 2.45.0
> 
> 
> ...Trying to follow this discussion across several threads and bug
> reports.   I was looped in when syzbot found that the [4] fix was a
> deadlock.
> 
> How are we reaching unmap_vmas() without the mmap lock held in any mode?
> We must be holding the read or write lock - otherwise the vma pointer is
> unsafe...?

The report was not calling unmap_vmas() but unmap_single_vma(), and this
patch proposed to move the untrack operation there.  We should always hold
write lock for unmap_vmas(), afaiu.

> 
> In any case, since this will just keep calling unmap_single_vma() it has
> to be an incomplete fix?

I think there's indeed some issue to settle besides this patch, however I
didn't quickly get why this patch is incomplete from this specific "untrack
pfn within unmap_single_vma()" problem.  I thought it was complete from
that regard, or could you elaborate otherwise?

For example, I think it's pretty common to use unmap_single_vma() in a
truncation path.

Thanks,

-- 
Peter Xu