On 14.12.22 21:04, Peter Xu wrote:
When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
have it and has pte marker installed. The warning is improper along with
the comment. The right thing is to inherit the pte marker when needed, or
keep the dst pte empty.
A vague guess is this happened by an accident when there's the prior patch
to introduce src/dst vma into this helper during the uffd-wp feature got
developed and I probably messed up in the rebase, since if we replace
dst_vma with src_vma the warning & comment it all makes sense too.
Hugetlb did exactly the right here (copy_hugetlb_page_range()). Fix the
general path.
Reproducer:
https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
Cc: <stable@xxxxxxxxxxxxxxx> # 5.19+
Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
Reported-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
---
mm/memory.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index aad226daf41b..032ef700c3e8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
return -EBUSY;
return -ENOENT;
} else if (is_pte_marker_entry(entry)) {
- /*
- * We're copying the pgtable should only because dst_vma has
- * uffd-wp enabled, do sanity check.
- */
- WARN_ON_ONCE(!userfaultfd_wp(dst_vma));
- set_pte_at(dst_mm, addr, dst_pte, pte);
+ if (userfaultfd_wp(dst_vma))
+ set_pte_at(dst_mm, addr, dst_pte, pte);
return 0;
}
if (!userfaultfd_wp(dst_vma))
Staring at the code first made me go "what about other PTE markers". I
then looked into the discussion in patch #2. The fix as is is
suboptimal, because it
1) Removes the warning which is good, but
2) Silently drops swapin errors now
So it silently breaks something else temporarily ...
I remember, that theoretically we could have multiple markers stored in
a single PTE marker.
Wouldn't it be cleaner to be able to "clean" specific markers from a PTE
marker without having to special case on each and everyone? I mean, only
uffd-wp is really special such that it might disappear for the target.
Something like (pseudocode):
if (!userfaultfd_wp(dst_vma))
pte_marker_clear_uff_wp(entry);
if (!pte_marker_empty(entry)) {
pte = make_pte_marker(pte_marker_get(entry));
set_pte_at(dst_mm, addr, dst_pte, pte);
}
Then this fix would be correct and backport-able even without #2. And it
would work for new types of markers :)
I'd prefer a fix that doesn't break something else temporarily, even if
the stable backport might require 5 additional minutes to do. So
squashing #2 into #1 would also work.
--
Thanks,
David / dhildenb
