Re: [PATCH 21/23] hugetlb/userfaultfd: Only drop uffd-wp special pte if required

[Peter Xu <peterx@redhat.com>]

On Fri, Apr 23, 2021 at 01:33:08PM -0700, Mike Kravetz wrote:
> On 3/22/21 5:50 PM, Peter Xu wrote:
> > Just like what we've done with shmem uffd-wp special ptes, we shouldn't drop
> > uffd-wp special swap pte for hugetlb too, only if we're going to unmap the
> > whole vma, or we're punching a hole with safe locks held.
> > 
> > For example, remove_inode_hugepages() is safe to drop uffd-wp ptes, because it
> > has taken hugetlb fault mutex so that no concurrent page fault would trigger.
> > While the call to hugetlb_vmdelete_list() in hugetlbfs_punch_hole() is not
> > safe.  That's why the previous call will be with ZAP_FLAG_DROP_FILE_UFFD_WP,
> > while the latter one won't be able to.
> 
> This commit message is a bit confusing, but the hugetlb hole punch code
> path is a bit confusing. :)   How about something like this?
> 
> As with  shmem uffd-wp special ptes, only drop the uffd-wp special swap
> pte if unmapping an entire vma or synchronized such that faults can not
> race with the unmap operation.  This requires passing zap_flags all the
> way to the lowest level hugetlb unmap routine: __unmap_hugepage_range.
> 
> In general, unmap calls originated in hugetlbfs code will pass the
> ZAP_FLAG_DROP_FILE_UFFD_WP flag as synchronization is in place to prevent
> faults.  The exception is hole punch which will first unmap without any
> synchronization.  Later when hole punch actually removes the page from
> the file, it will check to see if there was a subsequent fault and if
> so take the hugetlb fault mutex while unmapping again.  This second
> unmap will pass in ZAP_FLAG_DROP_FILE_UFFD_WP.

Sure, I can replace mine.

Maybe it's because I didn't explain enough on the reasoning so it's confusing.
The core justification of "whether to apply ZAP_FLAG_DROP_FILE_UFFD_WP flag
when unmap a hugetlb range" is (IMHO): we should never reach a state when a
page fault could errornously fault in a page-cache page that was wr-protected
to be writable, even in an extremely short period.  That could happen if
e.g. we pass ZAP_FLAG_DROP_FILE_UFFD_WP in hugetlbfs_punch_hole() when calling
hugetlb_vmdelete_list(), because if a page fault triggers after that call and
before the remove_inode_hugepages() right after it, the page cache can be
mapped writable again in the small window, which can cause data corruption.

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  fs/hugetlbfs/inode.c    | 15 +++++++++------
> >  include/linux/hugetlb.h | 13 ++++++++-----
> >  mm/hugetlb.c            | 27 +++++++++++++++++++++------
> >  mm/memory.c             |  5 ++++-
> >  4 files changed, 42 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index d81f52b87bd7..5fe19e801a2b 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -399,7 +399,8 @@ static void remove_huge_page(struct page *page)
> >  }
> >  
> >  static void
> > -hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end)
> > +hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end,
> > +		      unsigned long zap_flags)
> >  {
> >  	struct vm_area_struct *vma;
> >  
> > @@ -432,7 +433,7 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end)
> >  		}
> >  
> >  		unmap_hugepage_range(vma, vma->vm_start + v_offset, v_end,
> > -									NULL);
> > +				     NULL, zap_flags);
> >  	}
> >  }
> >  
> > @@ -513,7 +514,8 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
> >  				mutex_lock(&hugetlb_fault_mutex_table[hash]);
> >  				hugetlb_vmdelete_list(&mapping->i_mmap,
> >  					index * pages_per_huge_page(h),
> > -					(index + 1) * pages_per_huge_page(h));
> > +					(index + 1) * pages_per_huge_page(h),
> > +					ZAP_FLAG_DROP_FILE_UFFD_WP);
> >  				i_mmap_unlock_write(mapping);
> >  			}
> >  
> > @@ -579,7 +581,8 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
> >  	i_mmap_lock_write(mapping);
> >  	i_size_write(inode, offset);
> >  	if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
> > -		hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0);
> > +		hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0,
> > +				      ZAP_FLAG_DROP_FILE_UFFD_WP);
> >  	i_mmap_unlock_write(mapping);
> >  	remove_inode_hugepages(inode, offset, LLONG_MAX);
> >  }
> > @@ -612,8 +615,8 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
> >  		i_mmap_lock_write(mapping);
> >  		if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
> >  			hugetlb_vmdelete_list(&mapping->i_mmap,
> > -						hole_start >> PAGE_SHIFT,
> > -						hole_end  >> PAGE_SHIFT);
> > +					      hole_start >> PAGE_SHIFT,
> > +					      hole_end >> PAGE_SHIFT, 0);
> >  		i_mmap_unlock_write(mapping);
> >  		remove_inode_hugepages(inode, hole_start, hole_end);
> >  		inode_unlock(inode);
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 92710600596e..4047fa042782 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -121,14 +121,15 @@ long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *,
> >  			 unsigned long *, unsigned long *, long, unsigned int,
> >  			 int *);
> >  void unmap_hugepage_range(struct vm_area_struct *,
> > -			  unsigned long, unsigned long, struct page *);
> > +			  unsigned long, unsigned long, struct page *,
> > +			  unsigned long);
> >  void __unmap_hugepage_range_final(struct mmu_gather *tlb,
> >  			  struct vm_area_struct *vma,
> >  			  unsigned long start, unsigned long end,
> > -			  struct page *ref_page);
> > +			  struct page *ref_page, unsigned long zap_flags);
> >  void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >  				unsigned long start, unsigned long end,
> > -				struct page *ref_page);
> > +				struct page *ref_page, unsigned long zap_flags);
> 
> Nothing introduced with your patch, but it seems __unmap_hugepage_range_final
> does not need to be in the header and can be static in hugetlb.c.

It seems to be used in unmap_single_vma() of mm/memory.c?

> 
> Everything else looks good,
> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Please let me know whether you want my extra paragraph in the commit message,
then I'll coordinate accordingly with the R-b.  Thanks!

-- 
Peter Xu