| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190222153508.GE7783@redhat.com>
Date: Fri, 22 Feb 2019 10:35:09 -0500
From: Jerome Glisse <jglisse@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
David Hildenbrand <david@...hat.com>,
Hugh Dickins <hughd@...gle.com>,
Maya Gokhale <gokhale2@...l.gov>,
Pavel Emelyanov <xemul@...tuozzo.com>,
Johannes Weiner <hannes@...xchg.org>,
Martin Cracauer <cracauer@...s.org>, Shaohua Li <shli@...com>,
Marty McFadden <mcfadden8@...l.gov>,
Andrea Arcangeli <aarcange@...hat.com>,
Mike Kravetz <mike.kravetz@...cle.com>,
Denis Plotnikov <dplotnikov@...tuozzo.com>,
Mike Rapoport <rppt@...ux.vnet.ibm.com>,
Mel Gorman <mgorman@...e.de>,
"Kirill A . Shutemov" <kirill@...temov.name>,
"Dr . David Alan Gilbert" <dgilbert@...hat.com>
Subject: Re: [PATCH v2 14/26] userfaultfd: wp: handle COW properly for uffd-wp
On Fri, Feb 22, 2019 at 04:46:03PM +0800, Peter Xu wrote:
> On Thu, Feb 21, 2019 at 01:04:24PM -0500, Jerome Glisse wrote:
> > On Tue, Feb 12, 2019 at 10:56:20AM +0800, Peter Xu wrote:
> > > This allows uffd-wp to support write-protected pages for COW.
[...]
> > > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > > index 9d4433044c21..ae93721f3795 100644
> > > --- a/mm/mprotect.c
> > > +++ b/mm/mprotect.c
> > > @@ -77,14 +77,13 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> > > if (pte_present(oldpte)) {
> > > pte_t ptent;
> > > bool preserve_write = prot_numa && pte_write(oldpte);
> > > + struct page *page;
> > >
> > > /*
> > > * Avoid trapping faults against the zero or KSM
> > > * pages. See similar comment in change_huge_pmd.
> > > */
> > > if (prot_numa) {
> > > - struct page *page;
> > > -
> > > page = vm_normal_page(vma, addr, oldpte);
> > > if (!page || PageKsm(page))
> > > continue;
> > > @@ -114,6 +113,46 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> > > continue;
> > > }
> > >
> > > + /*
> > > + * Detect whether we'll need to COW before
> > > + * resolving an uffd-wp fault. Note that this
> > > + * includes detection of the zero page (where
> > > + * page==NULL)
> > > + */
> > > + if (uffd_wp_resolve) {
> > > + /* If the fault is resolved already, skip */
> > > + if (!pte_uffd_wp(*pte))
> > > + continue;
> > > + page = vm_normal_page(vma, addr, oldpte);
> > > + if (!page || page_mapcount(page) > 1) {
> >
> > This is wrong, if you allow page to be NULL then you gonna segfault
> > in wp_page_copy() down below. Are you sure you want to test for
> > special page ? For anonymous memory this should never happens ie
> > anon page always are regular page. So if you allow userfaulfd to
> > write protect only anonymous vma then there is no point in testing
> > here beside maybe a BUG_ON() just in case ...
>
> It's majorly for zero pages where page can be NULL. Would this be
> clearer:
>
> if (is_zero_pfn(pte_pfn(old_pte)) || (page && page_mapcount(page)))
>
> ?
>
> Now we treat zero pages as normal COW pages so we'll do COW here even
> for zero pages. I think maybe we can do special handling on all over
> the places for zero pages (e.g., we don't write protect a PTE if we
> detected that this is the zero PFN) but I'm uncertain on whether
> that's what we want, so I chose to start with current solution at
> least to achieve functionality first.
You can keep the vm_normal_page() in that case but split the if
between page == NULL and page != NULL with mapcount > 1. As other-
wise you will segfault below.
>
> >
> > > + struct vm_fault vmf = {
> > > + .vma = vma,
> > > + .address = addr & PAGE_MASK,
> > > + .page = page,
> > > + .orig_pte = oldpte,
> > > + .pmd = pmd,
> > > + /* pte and ptl not needed */
> > > + };
> > > + vm_fault_t ret;
> > > +
> > > + if (page)
> > > + get_page(page);
> > > + arch_leave_lazy_mmu_mode();
> > > + pte_unmap_unlock(pte, ptl);
> > > + ret = wp_page_copy(&vmf);
> > > + /* PTE is changed, or OOM */
> > > + if (ret == 0)
> > > + /* It's done by others */
> > > + continue;
> > > + else if (WARN_ON(ret != VM_FAULT_WRITE))
> > > + return pages;
> > > + pte = pte_offset_map_lock(vma->vm_mm,
> > > + pmd, addr,
> > > + &ptl);
> >
> > Here you remap the pte locked but you are not checking if the pte is
> > the one you expect ie is it pointing to the copied page and does it
> > have expect uffd_wp flag. Another thread might have raced between the
> > time you called wp_page_copy() and the time you pte_offset_map_lock()
> > I have not check the mmap_sem so maybe you are protected by it as
> > mprotect is taking it in write mode IIRC, if so you should add a
> > comments at very least so people do not see this as a bug.
>
> Thanks for spotting this. With nornal uffd-wp page fault handling
> path we're only with read lock held (and I would suspect it's racy
> even with write lock...). I agree that there can be a race right
> after the COW has done.
>
> Here IMHO we'll be fine as long as it's still a present PTE, in other
> words, we should be able to tolerate PTE changes as long as it's still
> present otherwise we'll need to retry this single PTE (e.g., the page
> can be quickly marked as migrating swap entry, or even the page could
> be freed beneath us). Do you think below change look good to you to
> be squashed into this patch?
Ok, but below if must be after arch_enter_lazy_mmu_mode(); not before.
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 73a65f07fe41..3423f9692838 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -73,6 +73,7 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> flush_tlb_batched_pending(vma->vm_mm);
> arch_enter_lazy_mmu_mode();
> do {
> +retry_pte:
> oldpte = *pte;
> if (pte_present(oldpte)) {
> pte_t ptent;
> @@ -149,6 +150,13 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> pte = pte_offset_map_lock(vma->vm_mm,
> pmd, addr,
> &ptl);
> + if (!pte_present(*pte))
> + /*
> + * This PTE could have
> + * been modified when COW;
> + * retry it
> + */
> + goto retry_pte;
> arch_enter_lazy_mmu_mode();
> }
> }
Powered by blists - more mailing lists