On Thu, Sep 13, 2018 at 10:23:28AM -0400, Jerome Glisse wrote: > On Thu, Sep 13, 2018 at 03:37:22PM +0800, Peter Xu wrote: > > On Wed, Sep 12, 2018 at 09:24:39AM -0400, Jerome Glisse wrote: > > > On Wed, Sep 12, 2018 at 09:03:55AM -0400, Jerome Glisse wrote: > > > > On Wed, Sep 12, 2018 at 02:49:21PM +0800, Peter Xu wrote: > > > > > Add an extra check on page dirty bit in change_pte_range() since there > > > > > might be case where PTE dirty bit is unset but it's actually dirtied. > > > > > One example is when a huge PMD is splitted after written: the dirty > > > > > bit > > > > > will be set on the compound page however we won't have the dirty bit > > > > > set > > > > > on each of the small page PTEs. > > > > > > > > > > I noticed this when debugging with a customized kernel that > > > > > implemented > > > > > userfaultfd write-protect. In that case, the dirty bit will be > > > > > critical > > > > > since that's required for userspace to handle the write protect page > > > > > fault (otherwise it'll get a SIGBUS with a loop of page faults). > > > > > However it should still be good even for upstream Linux to cover more > > > > > scenarios where we shouldn't need to do extra page faults on the small > > > > > pages if the previous huge page is already written, so the dirty bit > > > > > optimization path underneath can cover more. > > > > > > > > > > > > > So as said by Kirill NAK you are not looking at the right place for > > > > your bug please first apply the below patch and read my analysis in > > > > my last reply. > > > > > > Just to be clear you are trying to fix a userspace bug that is hidden > > > for non THP pages by a kernel space bug inside userfaultfd by making > > > the kernel space bug of userfaultfd buggy for THP too. > > > > > > > > > > > > > > Below patch fix userfaultfd bug. I am not posting it as it is on a > > > > branch and i am not sure when Andrea plan to post. Andrea feel free > > > > to squash that fix. > > > > > > > > > > > > From 35cdb30afa86424c2b9f23c0982afa6731be961c Mon Sep 17 00:00:00 2001 > > > > From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <[email protected]> > > > > Date: Wed, 12 Sep 2018 08:58:33 -0400 > > > > Subject: [PATCH] userfaultfd: do not set dirty accountable when changing > > > > protection > > > > MIME-Version: 1.0 > > > > Content-Type: text/plain; charset=UTF-8 > > > > Content-Transfer-Encoding: 8bit > > > > > > > > mwriteprotect_range() has nothing to do with the dirty accountable > > > > optimization so do not set it as it opens a door for userspace to > > > > unwrite protect pages in a range that is write protected ie the vma > > > > !(vm_flags & VM_WRITE). > > > > > > > > Signed-off-by: Jérôme Glisse <[email protected]> > > > > --- > > > > mm/userfaultfd.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > > > index a0379c5ffa7c..59db1ce48fa0 100644 > > > > --- a/mm/userfaultfd.c > > > > +++ b/mm/userfaultfd.c > > > > @@ -632,7 +632,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, > > > > unsigned long start, > > > > newprot = vm_get_page_prot(dst_vma->vm_flags); > > > > > > > > change_protection(dst_vma, start, start + len, newprot, > > > > - !enable_wp, 0); > > > > + false, 0); > > > > > > > > err = 0; > > > > out_unlock: > > > > Hi, Jerome, > > > > I tried your patch, unluckily it didn't work just like when not > > applied: > > > > Sep 13 15:16:52 px-ws kernel: FAULT_FLAG_ALLOW_RETRY missing 71 > > Sep 13 15:16:52 px-ws kernel: CPU: 5 PID: 1625 Comm: qemu-system-x86 Not > > tainted 4.19.0-rc2+ #31 > > > > Sep 13 15:16:52 px-ws kernel: Hardware name: LENOVO ThinkCentre > > M8500t-N000/SHARKBAY, BIOS FBKTC6AUS 06/22/2016 > > > > Sep 13 15:16:52 px-ws kernel: Call Trace: > > Sep 13 15:16:52 px-ws kernel: dump_stack+0x5c/0x7b > > Sep 13 15:16:52 px-ws kernel: handle_userfault+0x4b5/0x780 > > Sep 13 15:16:52 px-ws kernel: ? userfaultfd_ctx_put+0xb0/0xb0 > > Sep 13 15:16:52 px-ws kernel: do_wp_page+0x1bd/0x5a0 > > Sep 13 15:16:52 px-ws kernel: __handle_mm_fault+0x7f9/0x1250 > > Sep 13 15:16:52 px-ws kernel: handle_mm_fault+0xfc/0x1f0 > > Sep 13 15:16:52 px-ws kernel: __do_page_fault+0x255/0x520 > > Sep 13 15:16:52 px-ws kernel: do_page_fault+0x32/0x110 > > Sep 13 15:16:52 px-ws kernel: ? page_fault+0x8/0x30 > > Sep 13 15:16:52 px-ws kernel: page_fault+0x1e/0x30 > > Sep 13 15:16:52 px-ws kernel: RIP: 0033:0x7f2a9d3254e0 > > Sep 13 15:16:52 px-ws kernel: Code: 73 01 c1 ef 07 48 81 e6 00 f0 ff ff 81 > > e7 e0 1f 00 00 49 8d bc 3e 40 57 00 00 48 3b 37 48 8b f3 0f 85 a4 01 00 00 > > 48 03 77 10 <66> 89 06f > > Sep 13 15:16:52 px-ws kernel: RSP: 002b:00007f2ab1aae390 EFLAGS: 00010202 > > Sep 13 15:16:52 px-ws kernel: RAX: 0000000000000246 RBX: 0000000000001ff2 > > RCX: 0000000000000031 > > > > Sep 13 15:16:52 px-ws kernel: RDX: ffffffffffac9604 RSI: 00007f2a53e01ff2 > > RDI: 000055a98fa049c0 > > > > Sep 13 15:16:52 px-ws kernel: RBP: 0000000000001ff4 R08: 0000000000000000 > > R09: 0000000000000002 > > > > Sep 13 15:16:52 px-ws kernel: R10: 0000000000000000 R11: 00007f2a98201030 > > R12: 0000000000001ff2 > > > > Sep 13 15:16:52 px-ws kernel: R13: 0000000000000000 R14: 000055a98f9ff260 > > R15: 00007f2ab1aaf700 > > > > > > In case you'd like to try, here's the QEMU binary I'm testing: > > > > https://github.com/xzpeter/qemu/tree/peter-userfault-wp-test > > > > It write protects the whole system when received HMP command "info > > status" (I hacked that command for simplicity; it's of course not used > > for that...). > > > > Would you please help me understand how your patch could resolve the > > wp page fault from userspace if not with dirty_accountable set in the > > uffd-wp world (sorry for asking a question that is related to a custom > > tree, but finally it'll be targeted at upstream after all)? I asked > > this question in my previous reply to you in v1 but you didn't > > respond. I'd be glad to test any of your further patches if you can > > help solve the problem, but I'd also appreciate if you could explain > > it a bit on how it work since again I didn't see why it could work: > > again, if without that dirty_accountable set then IMO we will never > > setup _PAGE_WRITE for page entries and IMHO that's needed for > > resolving the page fault for uffd-wp tree. > > I missed that reply and forgot about PAGE_COPY ... So below is > what i believe a proper fix for your issue: >
Below is a slightly better one to avoid mkwrite on COW page but it is still kind of ugly to do that in those function maybe adding a new helper would be a better way dunno. Anyway untested but it is better than trying to set pte dirty. >From 8333ac9de1c54fe858f6f22dd21a2e14eebccd0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <[email protected]> Date: Thu, 13 Sep 2018 10:16:30 -0400 Subject: [PATCH] mm/mprotect: add a mkwrite paramater to change_protection() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mkwrite parameter allow to change read only pte to write one which is needed by userfaultfd to un-write-protect after a fault have been handled. Signed-off-by: Jérôme Glisse <[email protected]> --- include/linux/huge_mm.h | 2 +- include/linux/mm.h | 3 +- mm/huge_memory.c | 32 +++++++++++++++++++-- mm/mempolicy.c | 2 +- mm/mprotect.c | 61 +++++++++++++++++++++++++++++------------ mm/userfaultfd.c | 2 +- 6 files changed, 79 insertions(+), 23 deletions(-) diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index a8a126259bc4..b51ff7f8e65c 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -45,7 +45,7 @@ extern bool move_huge_pmd(struct vm_area_struct *vma, unsigned long old_addr, pmd_t *old_pmd, pmd_t *new_pmd, bool *need_flush); extern int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, unsigned long addr, pgprot_t newprot, - int prot_numa); + int prot_numa, bool mkwrite); int vmf_insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmd, pfn_t pfn, bool write); int vmf_insert_pfn_pud(struct vm_area_struct *vma, unsigned long addr, diff --git a/include/linux/mm.h b/include/linux/mm.h index 5d5c7fd07dc0..2bbf3e33bf9e 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1492,7 +1492,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma, bool need_rmap_locks); extern unsigned long change_protection(struct vm_area_struct *vma, unsigned long start, unsigned long end, pgprot_t newprot, - int dirty_accountable, int prot_numa); + int dirty_accountable, int prot_numa, + bool mkwrite); extern int mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev, unsigned long start, unsigned long end, unsigned long newflags); diff --git a/mm/huge_memory.c b/mm/huge_memory.c index abf621aba672..3038f4bd9cce 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1842,12 +1842,13 @@ bool move_huge_pmd(struct vm_area_struct *vma, unsigned long old_addr, * - HPAGE_PMD_NR is protections changed and TLB flush necessary */ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, - unsigned long addr, pgprot_t newprot, int prot_numa) + unsigned long addr, pgprot_t newprot, int prot_numa, + bool mkwrite) { struct mm_struct *mm = vma->vm_mm; spinlock_t *ptl; pmd_t entry; - bool preserve_write; + bool preserve_write, do_mkwrite = false; int ret; ptl = __pmd_trans_huge_lock(pmd, vma); @@ -1857,6 +1858,31 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, preserve_write = prot_numa && pmd_write(*pmd); ret = 1; + if (mkwrite && pmd_present(*pmd) && !pmd_write(*pmd)) { + struct page *page = pmd_page(pmd); + pmd_t orig_pmd = READ_ONCE(pmd); + + VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page); + /* + * We can only allow mkwrite if nobody else maps the huge page + * or it's part. + */ + if (!trylock_page(page)) { + get_page(page); + spin_unlock(ptl); + lock_page(page); + + ptl = __pmd_trans_huge_lock(pmd, vma); + if (!ptl) + return 0; + } + if (pmd_same(*pmd, orig_pmd) && reuse_swap_page(page, NULL)) { + do_mkwrite = true; + } + unlock_page(page); + put_page(page); + } + #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION if (is_swap_pmd(*pmd)) { swp_entry_t entry = pmd_to_swp_entry(*pmd); @@ -1925,6 +1951,8 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, entry = pmd_modify(entry, newprot); if (preserve_write) entry = pmd_mk_savedwrite(entry); + if (do_mkwrite) + entry = pmd_mkwrite(entry); ret = HPAGE_PMD_NR; set_pmd_at(mm, addr, pmd, entry); BUG_ON(vma_is_anonymous(vma) && !preserve_write && pmd_write(entry)); diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 4ce44d3ff03d..2d0ee09e6b26 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -579,7 +579,7 @@ unsigned long change_prot_numa(struct vm_area_struct *vma, { int nr_updated; - nr_updated = change_protection(vma, addr, end, PAGE_NONE, 0, 1); + nr_updated = change_protection(vma, addr, end, PAGE_NONE, 0, 1, false); if (nr_updated) count_vm_numa_events(NUMA_PTE_UPDATES, nr_updated); diff --git a/mm/mprotect.c b/mm/mprotect.c index 58b629bb70de..c43a29eebdaf 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -36,7 +36,7 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, unsigned long addr, unsigned long end, pgprot_t newprot, - int dirty_accountable, int prot_numa) + int dirty_accountable, int prot_numa, bool mkwrite) { struct mm_struct *mm = vma->vm_mm; pte_t *pte, oldpte; @@ -72,13 +72,15 @@ 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); + bool do_mkwrite = false; /* * Avoid trapping faults against the zero or KSM * pages. See similar comment in change_huge_pmd. */ - if (prot_numa) { + if (prot_numa || mkwrite) { struct page *page; + int tmp; page = vm_normal_page(vma, addr, oldpte); if (!page || PageKsm(page)) @@ -94,6 +96,26 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, */ if (target_node == page_to_nid(page)) continue; + + if (mkwrite) { + if (!trylock_page(page)) { + pte_t orig_pte = READ_ONCE(pte); + get_page(page); + pte_unmap_unlock(pte, ptl); + lock_page(page); + pte = pte_offset_map_lock(vma->vm_mm, pmd, + addr, &ptl); + if (!pte_same(*pte, orig_pte)) { + unlock_page(page); + put_page(page); + continue; + } + } + if (reuse_swap_page(vmf->page, &tmp)) + do_mkwrite = true; + unlock_page(page); + put_page(page); + } } ptent = ptep_modify_prot_start(mm, addr, pte); @@ -102,9 +124,9 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, ptent = pte_mk_savedwrite(ptent); /* Avoid taking write faults for known dirty pages */ - if (dirty_accountable && pte_dirty(ptent) && - (pte_soft_dirty(ptent) || - !(vma->vm_flags & VM_SOFTDIRTY))) { + if (do_mkwrite || (dirty_accountable && + pte_dirty(ptent) && (pte_soft_dirty(ptent) || + !(vma->vm_flags & VM_SOFTDIRTY)))) { ptent = pte_mkwrite(ptent); } ptep_modify_prot_commit(mm, addr, pte, ptent); @@ -150,7 +172,8 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, static inline unsigned long change_pmd_range(struct vm_area_struct *vma, pud_t *pud, unsigned long addr, unsigned long end, - pgprot_t newprot, int dirty_accountable, int prot_numa) + pgprot_t newprot, int dirty_accountable, int prot_numa, + bool mkwrite) { pmd_t *pmd; struct mm_struct *mm = vma->vm_mm; @@ -179,7 +202,7 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma, __split_huge_pmd(vma, pmd, addr, false, NULL); } else { int nr_ptes = change_huge_pmd(vma, pmd, addr, - newprot, prot_numa); + newprot, prot_numa, mkwrite); if (nr_ptes) { if (nr_ptes == HPAGE_PMD_NR) { @@ -194,7 +217,7 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma, /* fall through, the trans huge pmd just split */ } this_pages = change_pte_range(vma, pmd, addr, next, newprot, - dirty_accountable, prot_numa); + dirty_accountable, prot_numa, mkwrite); pages += this_pages; next: cond_resched(); @@ -210,7 +233,8 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma, static inline unsigned long change_pud_range(struct vm_area_struct *vma, p4d_t *p4d, unsigned long addr, unsigned long end, - pgprot_t newprot, int dirty_accountable, int prot_numa) + pgprot_t newprot, int dirty_accountable, int prot_numa, + bool mkwrite) { pud_t *pud; unsigned long next; @@ -222,7 +246,7 @@ static inline unsigned long change_pud_range(struct vm_area_struct *vma, if (pud_none_or_clear_bad(pud)) continue; pages += change_pmd_range(vma, pud, addr, next, newprot, - dirty_accountable, prot_numa); + dirty_accountable, prot_numa, mkwrite); } while (pud++, addr = next, addr != end); return pages; @@ -230,7 +254,8 @@ static inline unsigned long change_pud_range(struct vm_area_struct *vma, static inline unsigned long change_p4d_range(struct vm_area_struct *vma, pgd_t *pgd, unsigned long addr, unsigned long end, - pgprot_t newprot, int dirty_accountable, int prot_numa) + pgprot_t newprot, int dirty_accountable, int prot_numa, + bool mkwrite) { p4d_t *p4d; unsigned long next; @@ -242,7 +267,7 @@ static inline unsigned long change_p4d_range(struct vm_area_struct *vma, if (p4d_none_or_clear_bad(p4d)) continue; pages += change_pud_range(vma, p4d, addr, next, newprot, - dirty_accountable, prot_numa); + dirty_accountable, prot_numa, mkwrite); } while (p4d++, addr = next, addr != end); return pages; @@ -250,7 +275,7 @@ static inline unsigned long change_p4d_range(struct vm_area_struct *vma, static unsigned long change_protection_range(struct vm_area_struct *vma, unsigned long addr, unsigned long end, pgprot_t newprot, - int dirty_accountable, int prot_numa) + int dirty_accountable, int prot_numa, mkwrite) { struct mm_struct *mm = vma->vm_mm; pgd_t *pgd; @@ -267,7 +292,7 @@ static unsigned long change_protection_range(struct vm_area_struct *vma, if (pgd_none_or_clear_bad(pgd)) continue; pages += change_p4d_range(vma, pgd, addr, next, newprot, - dirty_accountable, prot_numa); + dirty_accountable, prot_numa, mkwrite); } while (pgd++, addr = next, addr != end); /* Only flush the TLB if we actually modified any entries: */ @@ -280,14 +305,16 @@ static unsigned long change_protection_range(struct vm_area_struct *vma, unsigned long change_protection(struct vm_area_struct *vma, unsigned long start, unsigned long end, pgprot_t newprot, - int dirty_accountable, int prot_numa) + int dirty_accountable, int prot_numa, bool mkwrite) { unsigned long pages; if (is_vm_hugetlb_page(vma)) pages = hugetlb_change_protection(vma, start, end, newprot); else - pages = change_protection_range(vma, start, end, newprot, dirty_accountable, prot_numa); + pages = change_protection_range(vma, start, end, newprot, + dirty_accountable, + prot_numa, mkwrite); return pages; } @@ -366,7 +393,7 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev, vma_set_page_prot(vma); change_protection(vma, start, end, vma->vm_page_prot, - dirty_accountable, 0); + dirty_accountable, 0, false); /* * Private VM_LOCKED VMA becoming writable: trigger COW to avoid major diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index a0379c5ffa7c..c745c5d87523 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -632,7 +632,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, newprot = vm_get_page_prot(dst_vma->vm_flags); change_protection(dst_vma, start, start + len, newprot, - !enable_wp, 0); + 0, 0, !enable_wp); err = 0; out_unlock: -- 2.17.1
