Messages in this thread | | | Date | Fri, 16 Dec 2022 10:04:27 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event |
| |
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@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); > 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
| |