On Mon, 12 Apr 2021, Axel Rasmussen wrote:

Minimizing header file inclusion is desirable. In this case, we can do so just by forward declaring the enumeration our signature relies upon.

Reviewed-by: Peter Xu peterx@redhat.com Signed-off-by: Axel Rasmussen axelrasmussen@google.com

---

include/linux/hugetlb.h | 4 +++- mm/hugetlb.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 09f1fd12a6fa..3f47650ab79b 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -11,7 +11,6 @@ #include <linux/kref.h> #include <linux/pgtable.h> #include <linux/gfp.h> -#include <linux/userfaultfd_k.h> struct ctl_table; struct user_struct; @@ -135,6 +134,8 @@ void hugetlb_show_meminfo(void); unsigned long hugetlb_total_pages(void); vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, unsigned long address, unsigned int flags);

+enum mcopy_atomic_mode;

Wrongly placed: the CONFIG_USERFAULTFD=y CONFIG_HUGETLB_PAGE=n build fails. Better place it up above with struct ctl_table etc.

#ifdef CONFIG_USERFAULTFD int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, pte_t *dst_pte, struct vm_area_struct *dst_vma, @@ -143,6 +144,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, pte_t *dst_pte, enum mcopy_atomic_mode mode, struct page **pagep); #endif /* CONFIG_USERFAULTFD */

bool hugetlb_reserve_pages(struct inode *inode, long from, long to, struct vm_area_struct *vma, vm_flags_t vm_flags); diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 54d81d5947ed..b1652e747318 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -40,6 +40,7 @@ #include <linux/hugetlb_cgroup.h> #include <linux/node.h> #include <linux/page_owner.h> +#include <linux/userfaultfd_k.h> #include "internal.h" int hugetlb_max_hstate __read_mostly; -- 2.31.1.295.g9ea45b61b8-goog

On Mon, 12 Apr 2021, Axel Rasmussen wrote:

Previously, we did a dance where we had one calling path in userfaultfd.c (mfill_atomic_pte), but then we split it into two in shmem_fs.h (shmem_{mcopy_atomic,mfill_zeropage}_pte), and then rejoined into a single shared function in shmem.c (shmem_mfill_atomic_pte).

This is all a bit overly complex. Just call the single combined shmem function directly, allowing us to clean up various branches, boilerplate, etc.

While we're touching this function, two other small cleanup changes:

• offset is equivalent to pgoff, so we can get rid of offset entirely.
• Split two VM_BUG_ON cases into two statements. This means the line number reported when the BUG is hit specifies exactly which condition was true.

Reviewed-by: Peter Xu peterx@redhat.com Signed-off-by: Axel Rasmussen axelrasmussen@google.com

Acked-by: Hugh Dickins hughd@google.com though you've dropped one minor fix I did like, see below...

---

include/linux/shmem_fs.h | 15 +++++------- mm/shmem.c | 52 +++++++++++++--------------------------- mm/userfaultfd.c | 10 +++----- 3 files changed, 25 insertions(+), 52 deletions(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index d82b6f396588..919e36671fe6 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -122,21 +122,18 @@ static inline bool shmem_file(struct file *file) extern bool shmem_charge(struct inode *inode, long pages); extern void shmem_uncharge(struct inode *inode, long pages); +#ifdef CONFIG_USERFAULTFD #ifdef CONFIG_SHMEM extern int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, struct vm_area_struct *dst_vma, unsigned long dst_addr, unsigned long src_addr,

• 		  bool zeropage,
  	  struct page **pagep);
  

-extern int shmem_mfill_zeropage_pte(struct mm_struct *dst_mm,

• 		    pmd_t *dst_pmd,
  

• 		    struct vm_area_struct *dst_vma,
  

• 		    unsigned long dst_addr);
  

-#else +#else /* !CONFIG_SHMEM */ #define shmem_mcopy_atomic_pte(dst_mm, dst_pte, dst_vma, dst_addr, \

In a previous version, you quietly corrected that "dst_pte" to "dst_pmd": of course it makes no difference to the code generated, but it was a good correction, helping to prevent confusion.

• 	       src_addr, pagep)        ({ BUG(); 0; })
  

-#define shmem_mfill_zeropage_pte(dst_mm, dst_pmd, dst_vma, \

• 		 dst_addr)      ({ BUG(); 0; })
  

-#endif

• 	       src_addr, zeropage, pagep)       ({ BUG(); 0; })
  

+#endif /* CONFIG_SHMEM */ +#endif /* CONFIG_USERFAULTFD */ #endif diff --git a/mm/shmem.c b/mm/shmem.c index 26c76b13ad23..b72c55aa07fc 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2354,13 +2354,14 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode return inode; } -static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,

• 		  pmd_t *dst_pmd,
  

• 		  struct vm_area_struct *dst_vma,
  

• 		  unsigned long dst_addr,
  

• 		  unsigned long src_addr,
  

• 		  bool zeropage,
  

• 		  struct page **pagep)
  

+#ifdef CONFIG_USERFAULTFD +int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm,

• 	   pmd_t *dst_pmd,
  

• 	   struct vm_area_struct *dst_vma,
  

• 	   unsigned long dst_addr,
  

• 	   unsigned long src_addr,
  

• 	   bool zeropage,
  

• 	   struct page **pagep)
  

{ struct inode *inode = file_inode(dst_vma->vm_file); struct shmem_inode_info *info = SHMEM_I(inode); @@ -2372,7 +2373,7 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, struct page *page; pte_t _dst_pte, *dst_pte; int ret;

• pgoff_t offset, max_off;

• pgoff_t max_off;

ret = -ENOMEM; if (!shmem_inode_acct_block(inode, 1)) @@ -2383,7 +2384,7 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, if (!page) goto out_unacct_blocks;

• if (!zeropage) {	/* mcopy_atomic */
  

• if (!zeropage) {	/* COPY */
  page_kaddr = kmap_atomic(page);
  ret = copy_from_user(page_kaddr,
  		     (const void __user *)src_addr,
  

@@ -2397,7 +2398,7 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, /* don't free the page */ return -ENOENT; }

• } else {		/* mfill_zeropage_atomic */
  

• } else {		/* ZEROPAGE */
  clear_highpage(page);
  

  } } else {

@@ -2405,15 +2406,15 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, *pagep = NULL; }

• VM_BUG_ON(PageLocked(page) || PageSwapBacked(page));

• VM_BUG_ON(PageLocked(page));
• VM_BUG_ON(PageSwapBacked(page)); __SetPageLocked(page); __SetPageSwapBacked(page); __SetPageUptodate(page);

ret = -EFAULT;

• offset = linear_page_index(dst_vma, dst_addr); max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);
• if (unlikely(offset >= max_off))

• if (unlikely(pgoff >= max_off)) goto out_release;

ret = shmem_add_to_page_cache(page, mapping, pgoff, NULL, @@ -2439,7 +2440,7 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, ret = -EFAULT; max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);

• if (unlikely(offset >= max_off))

• if (unlikely(pgoff >= max_off)) goto out_release_unlock;

ret = -EEXIST; @@ -2476,28 +2477,7 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, shmem_inode_unacct_blocks(inode, 1); goto out; }

-int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm,

• 	   pmd_t *dst_pmd,
  

• 	   struct vm_area_struct *dst_vma,
  

• 	   unsigned long dst_addr,
  

• 	   unsigned long src_addr,
  

• 	   struct page **pagep)
  

-{

• return shmem_mfill_atomic_pte(dst_mm, dst_pmd, dst_vma,

• 		      dst_addr, src_addr, false, pagep);
  

-}

-int shmem_mfill_zeropage_pte(struct mm_struct *dst_mm,

• 	     pmd_t *dst_pmd,
  

• 	     struct vm_area_struct *dst_vma,
  

• 	     unsigned long dst_addr)
  

-{

• struct page *page = NULL;
• return shmem_mfill_atomic_pte(dst_mm, dst_pmd, dst_vma,

• 		      dst_addr, 0, true, &page);
  

-} +#endif /* CONFIG_USERFAULTFD */ #ifdef CONFIG_TMPFS static const struct inode_operations shmem_symlink_inode_operations; diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index e14b3820c6a8..23fa2583bbd1 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -440,13 +440,9 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm, dst_vma, dst_addr); } else { VM_WARN_ON_ONCE(wp_copy);

• if (!zeropage)
  

• 	err = shmem_mcopy_atomic_pte(dst_mm, dst_pmd,
  

• 				     dst_vma, dst_addr,
  

• 				     src_addr, page);
  

• else
  

• 	err = shmem_mfill_zeropage_pte(dst_mm, dst_pmd,
  

• 				       dst_vma, dst_addr);
  

• err = shmem_mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,
  

• 			     dst_addr, src_addr, zeropage,
  

• 			     page);
  

  }

return err; -- 2.31.1.295.g9ea45b61b8-goog

On Wed, Apr 14, 2021 at 12:36:13AM -0700, Hugh Dickins wrote:

On Mon, 12 Apr 2021, Axel Rasmussen wrote:

...

This patch allows shmem-backed VMAs to be registered for minor faults. Minor faults are appropriately relayed to userspace in the fault path, for VMAs with the relevant flag.

This commit doesn't hook up the UFFDIO_CONTINUE ioctl for shmem-backed minor faults, though, so userspace doesn't yet have a way to resolve such faults.

This is a very odd way to divide up the series: an "Intermission" half way through the implementation of MINOR/CONTINUE: this 3/9 makes little sense without the 4/9 to mm/userfaultfd.c which follows.

But, having said that, I won't object and Peter did not object, and I don't know of anyone else looking here: it will only give each of us more trouble to insist on repartitioning the series, and it's the end state that's far more important to me and to all of us.

Agreed, ideally it should be after patch 4 since this patch enables the feature already.

And I'll even seize on it, to give myself an intermission after this one, until tomorrow (when I'll look at 4/9 and 9/9 - but shall not look at the selftests ones at all).

Most of this is okay, except the mm/shmem.c part; and I've just now realized that somewhere (whether in this patch or separately) there needs to be an update to Documentation/admin-guide/mm/userfaultfd.rst (admin-guide? how weird, but not this series' business to correct).

(maybe some dir "devel" would suite better? But I do also see soft-dirty.rst, idle_page_tracking.rst,..)

[...]

...

static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, @@ -1820,6 +1820,14 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, page = pagecache_get_page(mapping, index, FGP_ENTRY | FGP_HEAD | FGP_LOCK, 0);

• if (page && vma && userfaultfd_minor(vma)) {

• unlock_page(page);
  

• put_page(page);
  

• *fault_type = handle_userfault(vmf, VM_UFFD_MINOR);
  

• return 0;
  

• }

Okay, Peter persuaded you to move that up here: where indeed it does look better than the earlier "swapped" version.

But will crash on swap as it's currently written: it needs to say if (!xa_is_value(page)) { unlock_page(page); put_page(page); }

And this is definitely true... Thanks,

I did say before that it's more robust to return from the swap case after doing the shmem_swapin_page(). But I might be slowly realizing that the ioctl to add the pte (in 4/9) will do its shmem_getpage_gfp(), and that will bring in the swap if user did not already do so: so I was wrong to claim more robustness the other way, this placement should be fine. I think.

...

if (xa_is_value(page)) { error = shmem_swapin_page(inode, index, &page, sgp, gfp, vma, fault_type); -- 2.31.1.295.g9ea45b61b8-goog

On Mon, 12 Apr 2021, Axel Rasmussen wrote:

With this change, userspace can resolve a minor fault within a shmem-backed area with a UFFDIO_CONTINUE ioctl. The semantics for this match those for hugetlbfs - we look up the existing page in the page cache, and install PTEs for it.

s/PTEs/a PTE/

This commit introduces a new helper: mcopy_atomic_install_ptes.

The plural is misleading: it only installs a single pte, so I'm going to ask you to change it throughout to mcopy_atomic_install_pte() (I'm not thrilled with the "mcopy" nor the "atomic", but there you are being consistent with userfaultfd's peculiar naming, so let them be).

Why handle UFFDIO_CONTINUE for shmem in mm/userfaultfd.c, instead of in shmem.c? The existing userfault implementation only relies on shmem.c for VM_SHARED VMAs. However, minor fault handling / CONTINUE work just fine for !VM_SHARED VMAs as well. We'd prefer to handle CONTINUE for shmem in one place, regardless of shared/private (to reduce code duplication).

Why add a new mcopy_atomic_install_ptes helper? A problem we have with continue is that shmem_mcopy_atomic_pte() and mcopy_atomic_pte() are *close* to what we want, but not exactly. We do want to setup the PTEs in a CONTINUE operation, but we don't want to e.g. allocate a new page, charge it (e.g. to the shmem inode), manipulate various flags, etc. Also we have the problem stated above: shmem_mcopy_atomic_pte() and mcopy_atomic_pte() both handle one-half of the problem (shared / private) continue cares about. So, introduce mcontinue_atomic_pte(), to handle all of the shmem continue cases. Introduce the helper so it doesn't duplicate code with mcopy_atomic_pte().

In a future commit, shmem_mcopy_atomic_pte() will also be modified to use this new helper. However, since this is a bigger refactor, it seems most clear to do it as a separate change.

(Actually that turns out to be a nice deletion of lines, but you're absolutely right to do it as a separate patch.)

Signed-off-by: Axel Rasmussen axelrasmussen@google.com

mm/userfaultfd.c | 176 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 131 insertions(+), 45 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 23fa2583bbd1..8df0438f5d6a 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -48,6 +48,87 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm, return dst_vma; } +/*

• • Install PTEs, to map dst_addr (within dst_vma) to page.
• • This function handles MCOPY_ATOMIC_CONTINUE (which is always file-backed),
• • whether or not dst_vma is VM_SHARED. It also handles the more general
• • MCOPY_ATOMIC_NORMAL case, when dst_vma is *not* VM_SHARED (it may be file
• • backed, or not).
• • Note that MCOPY_ATOMIC_NORMAL for a VM_SHARED dst_vma is handled by
• • shmem_mcopy_atomic_pte instead.

Right, I'm thinking in terms of five cases below (I'm not for a moment saying that you need to list these out in the comment, just saying that I could not get my head around the issues in this function without listing them out for myself):

1. anon private mcopy (using anon page newly allocated) 2. shmem private mcopy (using anon page newly allocated) 3. shmem private mcontinue (using page in cache from shmem_getpage) 4. shmem shared mcontinue (using page in cache from shmem_getpage) 5. shmem shared mcopy (using page in cache newly allocated)

Of which each has a VM_WRITE and a !VM_WRITE case; and the third and fourth cases are new in this patch (it really would have been better to introduce mcopy_atomic_install_pte() in a separate earlier patch, but don't change that now we've got this far); and the fifth case does *not* use mcopy_atomic_install_pte() in this patch, but will in future.

And while making these notes, let's hightlight again what is commented elsewhere, the odd nature of the second case: where userfaultfd short circuits to an anonymous CoW page without instantiating the shmem page. (Please double-check me on that: quite a lot of my comments below are about this case 2, so if I've got it wrong, then I've got a lot wrong.)

• */

+static int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd,

mcopy_atomic_install_pte() throughout please.

• 		     struct vm_area_struct *dst_vma,
  

• 		     unsigned long dst_addr, struct page *page,
  

• 		     bool newly_allocated, bool wp_copy)
  

+{

• int ret;
• pte_t _dst_pte, *dst_pte;
• int writable;

Sorry, it's silly of me, but I keep getting irritated by "int writable" in company with the various bools; and the way vm_shared is initialized below, but writable initialized later. Please humour me by making it bool writable = dst_vma->vm_flags & VM_WRITE;

• bool vm_shared = dst_vma->vm_flags & VM_SHARED;

And I've found that we also need bool page_in_cache = page->mapping; because an anonymous page does not at this point have page->mapping set, and does not yet satisfy PageAnon(page). Or other naming if you think of better; or its inverse with page_is_anon or whatever.

• spinlock_t *ptl;
• struct inode *inode;
• pgoff_t offset, max_off;
• _dst_pte = mk_pte(page, dst_vma->vm_page_prot);
• writable = dst_vma->vm_flags & VM_WRITE;
• /* For private, non-anon we need CoW (don't write to page cache!) */
• if (!vma_is_anonymous(dst_vma) && !vm_shared)

• writable = 0;
  

That appears to differ slightly from what was done before this patch: it is now making a case 2 VM_WRITE pte unwritable, incurring an unnecessary write fault later on. I think it would be better (for all fives cases) to say:

if (page_in_cache && !vm_shared) writable = false;

• if (writable || vma_is_anonymous(dst_vma))

• _dst_pte = pte_mkdirty(_dst_pte);
  

And, unlike before, that is not marking the case 2 unwritable pte dirty. Which works okay, because add_to_swap()'s unconditional set_page_dirty() will make sure this page is written to swap before it is freed. But I'd rather not rely on that here: it's a detail which might get changed one day, and whoever changes it may not think to update mm/userfaultfd.c.

Sticking with Andrea's caution about marking a shared unwritable dirty, but happy as before to mark a private unwritable dirty:

if (writable || !page_in_cache) _dst_pte = pte_mkdirty(_dst_pte);

This does *not* mark the new cases 3 and 4 dirty when unwritable, but there's no chance of data loss in their case, because the kernel has not modified the page's data: the page from shmem_getpage() is already marked correctly (usually PageDirty, but there's a mapped-hole case where it might not be, and that is still correct).

(Why do we mark these pages dirty when writable? To skip a hardware fault when and if the page is written later; but I'm not sure whether that's necessarily a good idea - we don't know here whether it was a write fault which triggered all this. I also don't know what difference wp_copy, which skips the mkwrite, makes to this calculus; but follow the example of before.)

• if (writable) {

• if (wp_copy)
  

• 	_dst_pte = pte_mkuffd_wp(_dst_pte);
  

• else
  

• 	_dst_pte = pte_mkwrite(_dst_pte);
  

• }

Fine.

else if (vm_shared) {

• /*
  

•  * Since we didn't pte_mkdirty(), mark the page dirty or it
  

•  * could be freed from under us. We could do this
  

•  * unconditionally, but doing it only if !writable is faster.
  

•  */
  

• set_page_dirty(page);
  

• }

But delete this block, as we all wanted. As I've argued above, the new cases 3 and 4 using shmem_getpage() do not need an extra set_page_dirty() here, and we can address case 5 when it's added.

• dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
• if (vma_is_shmem(dst_vma)) {

• /* serialize against truncate with the page table lock */
  

• inode = dst_vma->vm_file->f_inode;
  

• offset = linear_page_index(dst_vma, dst_addr);
  

• max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);
  

• ret = -EFAULT;
  

• if (unlikely(offset >= max_off))
  

• 	goto out_unlock;
  

• }
• ret = -EEXIST;
• if (!pte_none(*dst_pte))

• goto out_unlock;
  

• inc_mm_counter(dst_mm, mm_counter(page));

Hard to spot, but that's wrong: because mm_counter() depends on PageAnon to decide which count to adjust, and that does not get set until the page_add_new_anon_rmap(). I'd expect your tests to have left "Bad rss" warnings in the kernel log? This would be why. Just move the line down until after page_add_new_anon_rmap() - with a comment line to say why!

• if (vma_is_shmem(dst_vma))

No, that gets case 2 wrong: use "if (page_in_cache)" instead.

• page_add_file_rmap(page, false);
  

• else

• page_add_new_anon_rmap(page, dst_vma, dst_addr, false);
  

• if (newly_allocated)

• lru_cache_add_inactive_or_unevictable(page, dst_vma);
  

• set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
• /* No need to invalidate - it was non-present before */
• update_mmu_cache(dst_vma, dst_addr, dst_pte);
• ret = 0;

+out_unlock:

• pte_unmap_unlock(dst_pte, ptl);
• return ret;

+}

static int mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, struct vm_area_struct *dst_vma, @@ -56,13 +137,9 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm, struct page **pagep, bool wp_copy) {

• pte_t _dst_pte, *dst_pte;
• spinlock_t *ptl; void *page_kaddr; int ret; struct page *page;
• pgoff_t offset, max_off;
• struct inode *inode;

if (!*pagep) { ret = -ENOMEM; @@ -99,43 +176,12 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm, if (mem_cgroup_charge(page, dst_mm, GFP_KERNEL)) goto out_release;

• _dst_pte = pte_mkdirty(mk_pte(page, dst_vma->vm_page_prot));
• if (dst_vma->vm_flags & VM_WRITE) {

• if (wp_copy)
  

• 	_dst_pte = pte_mkuffd_wp(_dst_pte);
  

• else
  

• 	_dst_pte = pte_mkwrite(_dst_pte);
  

• }
• dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
• if (dst_vma->vm_file) {

• /* the shmem MAP_PRIVATE case requires checking the i_size */
  

• inode = dst_vma->vm_file->f_inode;
  

• offset = linear_page_index(dst_vma, dst_addr);
  

• max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);
  

• ret = -EFAULT;
  

• if (unlikely(offset >= max_off))
  

• 	goto out_release_uncharge_unlock;
  

• }
• ret = -EEXIST;
• if (!pte_none(*dst_pte))

• goto out_release_uncharge_unlock;
  

• inc_mm_counter(dst_mm, MM_ANONPAGES);
• page_add_new_anon_rmap(page, dst_vma, dst_addr, false);
• lru_cache_add_inactive_or_unevictable(page, dst_vma);
• set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
• /* No need to invalidate - it was non-present before */
• update_mmu_cache(dst_vma, dst_addr, dst_pte);
• pte_unmap_unlock(dst_pte, ptl);
• ret = 0;

• ret = mcopy_atomic_install_ptes(dst_mm, dst_pmd, dst_vma, dst_addr,

• 			page, true, wp_copy);
  

• if (ret)

• goto out_release;
  

out: return ret; -out_release_uncharge_unlock:

• pte_unmap_unlock(dst_pte, ptl);

out_release: put_page(page); goto out; @@ -176,6 +222,41 @@ static int mfill_zeropage_pte(struct mm_struct *dst_mm, return ret; } +/* Handles UFFDIO_CONTINUE for all shmem VMAs (shared or private). */ +static int mcontinue_atomic_pte(struct mm_struct *dst_mm,

• 		pmd_t *dst_pmd,
  

• 		struct vm_area_struct *dst_vma,
  

• 		unsigned long dst_addr,
  

• 		bool wp_copy)
  

+{

• struct inode *inode = file_inode(dst_vma->vm_file);
• pgoff_t pgoff = linear_page_index(dst_vma, dst_addr);
• struct page *page;
• int ret;
• ret = shmem_getpage(inode, pgoff, &page, SGP_READ);
• if (ret)

• goto out;
  

• if (!page) {

• ret = -EFAULT;
  

• goto out;
  

• }

Right, I'll go along with that. I did say to use SGP_CACHE, and I'm not sure why you did not, but perhaps were put off it by my remarks about a racing hole punch. Using SGP_READ here, you will not allocate an unnecessary page in that (exceptional) case, good; but you are left with inconsistent behaviour on fallocated (!PageUptodate: the page has been allocated, but not yet cleared or overwritten with user data) pages.

No bad data is leaked, but the inconsistency is that handle_userfault() believes there's a page at this offset, but mcontinue_atomic_pte() says there is not (and might they retry forever disagreeing?). It's a somewhat grey area: I'd say your mcontinue_atomic_pte() is the correct one (it is a hole, but one that we happen to have reserved future space for); but that we don't really want to complicate the other end for it (if we skip going the VM_UFFD_MINOR way, it's more of a problem for VM_UFFD_MISSING).

I think stick with SGP_READ as you have: just be aware at the userspace end that this case might occur (and you only have to fault the page into the other mapping to resolve it), if anyone is using fallocate().

All the rest of 4/9 looked fine to me, though I have worried about a couple more things.

One: whereas I tend to think of one call to handle_userfault() ending up in one call to mcopy_atomic_install_pte() to resolve it, I see that actually __mcopy_atomic() can be a loop over many pages, and I have not thought through all the possibilities that might allow, and now with the interspersal of MINOR and MISSING.

Two: mcopy_atomic_install_pte() can only install a pte, and it looks as if it handles tails of a compound page correctly (as might come from a MINOR userfault on a pre-existing shmem THP); but there is no mapping of huge page by pmd, and khugepaged's userfaultfd_armed() checks will (rightly) keep it from interfering. I guess later on, after all the userfaultfd-ing is done, khugepaged can come around and collapse to huge pages, if the file was on a huge mount: okay.

Hugh

• ret = mcopy_atomic_install_ptes(dst_mm, dst_pmd, dst_vma, dst_addr,

• 			page, false, wp_copy);
  

• if (ret)

• goto out_release;
  

• unlock_page(page);
• ret = 0;

+out:

• return ret;

+out_release:

• unlock_page(page);
• put_page(page);
• goto out;

+}

static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address) { pgd_t *pgd; @@ -415,11 +496,16 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm, unsigned long dst_addr, unsigned long src_addr, struct page **page,

• 				bool zeropage,
  

• 				enum mcopy_atomic_mode mode,
  			bool wp_copy)
  

{ ssize_t err;

• if (mode == MCOPY_ATOMIC_CONTINUE) {

• return mcontinue_atomic_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
  

• 			    wp_copy);
  

• }
• /*
  • The normal page fault path for a shmem will invoke the
  • fault, fill the hole in the file and COW it right away. The

@@ -431,7 +517,7 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm, * and not in the radix tree. */ if (!(dst_vma->vm_flags & VM_SHARED)) {

• if (!zeropage)
  

• if (mode == MCOPY_ATOMIC_NORMAL)
  err = mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,
  		       dst_addr, src_addr, page,
  		       wp_copy);
  

@@ -441,7 +527,8 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm, } else { VM_WARN_ON_ONCE(wp_copy); err = shmem_mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,

• 			     dst_addr, src_addr, zeropage,
  

• 			     dst_addr, src_addr,
  

• 			     mode != MCOPY_ATOMIC_NORMAL,
  		     page);
  

  }

@@ -463,7 +550,6 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, long copied; struct page *page; bool wp_copy;

• bool zeropage = (mcopy_mode == MCOPY_ATOMIC_ZEROPAGE);

/* * Sanitize the command parameters: @@ -526,7 +612,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, if (!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma)) goto out_unlock;

• if (mcopy_mode == MCOPY_ATOMIC_CONTINUE)

• if (!vma_is_shmem(dst_vma) && mcopy_mode == MCOPY_ATOMIC_CONTINUE) goto out_unlock;

/* @@ -574,7 +660,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, BUG_ON(pmd_trans_huge(*dst_pmd)); err = mfill_atomic_pte(dst_mm, dst_pmd, dst_vma, dst_addr,

• 		       src_addr, &page, zeropage, wp_copy);
  

• 		       src_addr, &page, mcopy_mode, wp_copy);
  

  cond_resched();

if (unlikely(err == -ENOENT)) { -- 2.31.1.295.g9ea45b61b8-goog

On Tue, Apr 13, 2021 at 1:15 PM Peter Xu peterx@redhat.com wrote:

On Mon, Apr 12, 2021 at 10:17:19PM -0700, Axel Rasmussen wrote:

...

Currently, the context (fds, mmap-ed areas, etc.) are global. Each test mutates this state in some way, in some cases really "clobbering it" (e.g., the events test mremap-ing area_dst over the top of area_src, or the minor faults tests overwriting the count_verify values in the test areas). We run the tests in a particular order, each test is careful to make the right assumptions about its starting state, etc.

But, this is fragile. It's better for a test's success or failure to not depend on what some other prior test case did to the global state.

To that end, clear and reinitialize the test context at the start of each test case, so whatever prior test cases did doesn't affect future tests.

This is particularly relevant to this series because the events test's mremap of area_dst screws up assumptions the minor fault test was relying on. This wasn't a problem for hugetlb, as we don't mremap in that case.

Signed-off-by: Axel Rasmussen axelrasmussen@google.com

tools/testing/selftests/vm/userfaultfd.c | 221 +++++++++++++---------- 1 file changed, 127 insertions(+), 94 deletions(-)

diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c index 1f65c4ab7994..0ff01f437a39 100644 --- a/tools/testing/selftests/vm/userfaultfd.c +++ b/tools/testing/selftests/vm/userfaultfd.c @@ -89,7 +89,8 @@ static int shm_fd; static int huge_fd; static char *huge_fd_off0; static unsigned long long *count_verify; -static int uffd, uffd_flags, finished, *pipefd; +static int uffd = -1; +static int uffd_flags, finished, *pipefd; static char *area_src, *area_src_alias, *area_dst, *area_dst_alias; static char *zeropage; pthread_attr_t attr; @@ -342,6 +343,121 @@ static struct uffd_test_ops hugetlb_uffd_test_ops = {

static struct uffd_test_ops *uffd_test_ops;

+static int userfaultfd_open(uint64_t *features) +{

• struct uffdio_api uffdio_api;
  

• uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
  

Keep UFFD_USER_MODE_ONLY?

[...]

...

@@ -961,10 +1045,9 @@ static int userfaultfd_zeropage_test(void) printf("testing UFFDIO_ZEROPAGE: "); fflush(stdout);

• uffd_test_ops->release_pages(area_dst);
  

• if (userfaultfd_open(0))
  

• if (uffd_test_ctx_clear() || uffd_test_ctx_init(0))
          return 1;
  

Would it look even nicer to init() at the entry of each test, and clear() after finish one test?

I slightly prefer clearing at the beginning, as it means we don't need to depend on the previous test being correct for this test to function. And, we don't need more complex error handling in the test cases to make sure we don't mess things up for the next test.

But, two things we can do to clean this up as-is:

The initialization function can just call clear itself, so tests don't need to worry about it.

And, with err(), we don't need these functions to return an int any more.

I'll send a version like that, we can see how it looks.

...

• uffdio_register.range.start = (unsigned long) area_dst;
  uffdio_register.range.len = nr_pages * page_size;
  uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
  

The rest looks good to me. Thanks,

-- Peter Xu

On Fri, Apr 16, 2021 at 5:34 PM Hugh Dickins hughd@google.com wrote:

On Mon, 12 Apr 2021, Axel Rasmussen wrote:

...

In a previous commit, we added the mcopy_atomic_install_ptes() helper. This helper does the job of setting up PTEs for an existing page, to map it into a given VMA. It deals with both the anon and shmem cases, as well as the shared and private cases.

In other words, shmem_mcopy_atomic_pte() duplicates a case it already handles. So, expose it, and let shmem_mcopy_atomic_pte() use it directly, to reduce code duplication.

This requires that we refactor shmem_mcopy_atomic-pte() a bit:

Instead of doing accounting (shmem_recalc_inode() et al) part-way through the PTE setup, do it beforehand. This frees up mcopy_atomic_install_ptes() from having to care about this accounting, but it does mean we need to clean it up if we get a failure afterwards (shmem_uncharge()).

We can *almost* use shmem_charge() to do this, reducing code duplication. But, it does `inode->i_mapping->nrpages++`, which would double-count since shmem_add_to_page_cache() also does this.

Signed-off-by: Axel Rasmussen axelrasmussen@google.com

include/linux/userfaultfd_k.h | 5 ++++ mm/shmem.c | 52 +++++++---------------------------- mm/userfaultfd.c | 25 ++++++++--------- 3 files changed, 27 insertions(+), 55 deletions(-)

Very nice, and it gets better.

...

diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index 794d1538b8ba..3e20bfa9ef80 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -53,6 +53,11 @@ enum mcopy_atomic_mode { MCOPY_ATOMIC_CONTINUE, };

+extern int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd,

mcopy_atomic_install_pte throughout as before.

...

•                              struct vm_area_struct *dst_vma,
  

•                              unsigned long dst_addr, struct page *page,
  

•                              bool newly_allocated, bool wp_copy);
  

extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start, unsigned long src_start, unsigned long len, bool *mmap_changing, __u64 mode); diff --git a/mm/shmem.c b/mm/shmem.c index 3f48cb5e8404..9b12298405a4 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2376,10 +2376,8 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, struct address_space *mapping = inode->i_mapping; gfp_t gfp = mapping_gfp_mask(mapping); pgoff_t pgoff = linear_page_index(dst_vma, dst_addr);

• spinlock_t *ptl;
  void *page_kaddr;
  struct page *page;
  

• pte_t _dst_pte, *dst_pte;
  int ret;
  pgoff_t max_off;
  
  

@@ -2389,8 +2387,10 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm,

  if (!*pagep) {
          page = shmem_alloc_page(gfp, info, pgoff);

•         if (!page)
  

•                 goto out_unacct_blocks;
  

•         if (!page) {
  

•                 shmem_inode_unacct_blocks(inode, 1);
  

•                 goto out;
  

•         }
  
          if (!zeropage) {        /* COPY */
                  page_kaddr = kmap_atomic(page);
  

@@ -2430,59 +2430,27 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, if (ret) goto out_release;

• _dst_pte = mk_pte(page, dst_vma->vm_page_prot);
  

• if (dst_vma->vm_flags & VM_WRITE)
  

•         _dst_pte = pte_mkwrite(pte_mkdirty(_dst_pte));
  

• else {
  

•         /*
  

•          * We don't set the pte dirty if the vma has no
  

•          * VM_WRITE permission, so mark the page dirty or it
  

•          * could be freed from under us. We could do it
  

•          * unconditionally before unlock_page(), but doing it
  

•          * only if VM_WRITE is not set is faster.
  

•          */
  

•         set_page_dirty(page);
  

• }
  

• dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
  

• ret = -EFAULT;
  

• max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);
  

• if (unlikely(pgoff >= max_off))
  

•         goto out_release_unlock;
  

• ret = -EEXIST;
  

• if (!pte_none(*dst_pte))
  

•         goto out_release_unlock;
  

• lru_cache_add(page);
  

• spin_lock_irq(&info->lock);
  info->alloced++;
  inode->i_blocks += BLOCKS_PER_PAGE;
  shmem_recalc_inode(inode);
  spin_unlock_irq(&info->lock);
  
  

• inc_mm_counter(dst_mm, mm_counter_file(page));
  

• page_add_file_rmap(page, false);
  

• set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
  

• ret = mcopy_atomic_install_ptes(dst_mm, dst_pmd, dst_vma, dst_addr,
  

•                                 page, true, false);
  

• if (ret)
  

•         goto out_release_uncharge;
  
  

• /* No need to invalidate - it was non-present before */
  

• update_mmu_cache(dst_vma, dst_addr, dst_pte);
  

• pte_unmap_unlock(dst_pte, ptl);
  

In reviewing 4/9, I said to take out mcopy_atomic_install_pte()'s set_page_dirty(). Just call it here instead, before the unlock_page(). You have an array of choices for how to write it:

    if (!(dst_vma->vm_flags & VM_WRITE))
            set_page_dirty(page);

or if (!(dst_vma->vm_flags & VM_WRITE)) SetPageDirty(page); or set_page_dirty(page); or SetPageDirty(page);

Personally, I'd go for the last: this function has just modified the page, so it ought to mark it dirty: without second-guessing what mcopy_atomic_install_pte() might do to make that redundant in the VM_WRITE case. set_page_dirty() or SetPageDirty()? Some years ago I tended to favour the former for its preparatory PageDirty check; nowadays (on shmem) I favour the latter, to avoid the function call indirection which became more expensive with spectre+retpoline.

Now reaching this comment after looking through patch 4:

Ah, this makes sense. I agree this is clearer too, as it doesn't split the concern across two files / functions.

...

  unlock_page(page);
  ret = 0;

out: return ret; -out_release_unlock:

• pte_unmap_unlock(dst_pte, ptl);
  

• ClearPageDirty(page);
  

+out_release_uncharge:

Given how 4/9 was, this did still need the ClearPageDirty() before delete_from_page_cache(), to prevent a warning. But not needed here if SPD is done just before unlock_page().

...

  delete_from_page_cache(page);

• shmem_uncharge(inode, 1);
  

out_release: unlock_page(page); put_page(page); -out_unacct_blocks:

• shmem_inode_unacct_blocks(inode, 1);
  goto out;
  

} #endif /* CONFIG_USERFAULTFD */ diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 8df0438f5d6a..3f73ba0b99f0 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -51,18 +51,13 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm, /*

• Install PTEs, to map dst_addr (within dst_vma) to page.

• • This function handles MCOPY_ATOMIC_CONTINUE (which is always file-backed),
• • whether or not dst_vma is VM_SHARED. It also handles the more general
• • MCOPY_ATOMIC_NORMAL case, when dst_vma is *not* VM_SHARED (it may be file
• • backed, or not).
• • Note that MCOPY_ATOMIC_NORMAL for a VM_SHARED dst_vma is handled by
• • shmem_mcopy_atomic_pte instead.

• • This function handles both MCOPY_ATOMIC_NORMAL and _CONTINUE for both shmem
• • and anon, and for both shared and private VMAs.
  */

-static int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd,

•                              struct vm_area_struct *dst_vma,
  

•                              unsigned long dst_addr, struct page *page,
  

•                              bool newly_allocated, bool wp_copy)
  

+int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd,

•                       struct vm_area_struct *dst_vma,
  

•                       unsigned long dst_addr, struct page *page,
  

•                       bool newly_allocated, bool wp_copy)
  

{ int ret; pte_t _dst_pte, *dst_pte; @@ -116,8 +111,12 @@ static int mcopy_atomic_install_ptes(struct mm_struct *dst_mm, pmd_t *dst_pmd, else page_add_new_anon_rmap(page, dst_vma, dst_addr, false);

• if (newly_allocated)
  

•         lru_cache_add_inactive_or_unevictable(page, dst_vma);
  

• if (newly_allocated) {
  

•         if (vma_is_shmem(dst_vma) && vm_shared)
  

•                 lru_cache_add(page);
  

•         else
  

•                 lru_cache_add_inactive_or_unevictable(page, dst_vma);
  

• }
  

This change is not required, you'll be glad to hear. Take a look at lru_cache_add_inactive_or_unevictable() in mm/swap.c: it's a wrapper to set PageMlocked and do associated accounting (if appropriate) before doing the lru_cache_add(page). And, strictly speaking, should have been used in shmem_mcopy_atomic_pte() all along.

I say "strictly speaking" because it would be exceptional to find a page needing Mlock there, and not an error to delay that Mlock until sometime later, before reclaim. Worth a sentence in the commit message, but not worth a "Fixes:" tag.

This makes sense. I had considered doing this before, but took a brief look at the functions and saw they were indeed different. But looking more closely, you're right that the wrapper is only different if some particular conditions are true, and so is fine to use in other cases.

...

  set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);

-- 2.31.1.295.g9ea45b61b8-goog