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...
Cc: stable@vger.kernel.org # 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@intel.com Signed-off-by: Peter Xu peterx@redhat.com
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);
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.