Re: [PATCH 20/35] mm: Update can_follow_write_pte() for shadow stack
From: Dave Hansen <hidden>
Date: 2022-02-09 22:50:42
Also in:
linux-arch, linux-doc, linux-mm, lkml
On 1/30/22 13:18, Rick Edgecombe wrote:
From: Yu-cheng Yu <redacted> Can_follow_write_pte() ensures a read-only page is COWed by checking the FOLL_COW flag, and uses pte_dirty() to validate the flag is still valid. Like a writable data page, a shadow stack page is writable, and becomes read-only during copy-on-write,
I thought we could not have read-only shadow stack pages. What does a read-only shadow stack PTE look like? ;)
but it is always dirty. Thus, in the can_follow_write_pte() check, it belongs to the writable page case and should be excluded from the read-only page pte_dirty() check. Apply the same changes to can_follow_write_pmd(). While at it, also split the long line into smaller ones.
FWIW, I probably would have had a preparatory patch for this part. The advantage is that if you break existing code, it's a lot easier to figure it out if you have a separate refactoring patch. Also, for a patch like this, the refactoring might result in the same exact binary. It's a pretty good sign that your patch won't cause regressions if it results in the same binary.
quoted hunk ↗ jump to hunk
diff --git a/mm/gup.c b/mm/gup.c index f0af462ac1e2..95b7d1084c44 100644 --- a/mm/gup.c +++ b/mm/gup.c@@ -464,10 +464,18 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address, * FOLL_FORCE can write to even unwritable pte's, but only * after we've gone through a COW cycle and they are dirty. */ -static inline bool can_follow_write_pte(pte_t pte, unsigned int flags) +static inline bool can_follow_write_pte(pte_t pte, unsigned int flags, + struct vm_area_struct *vma) { - return pte_write(pte) || - ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_dirty(pte)); + if (pte_write(pte)) + return true; + if ((flags & (FOLL_FORCE | FOLL_COW)) != (FOLL_FORCE | FOLL_COW)) + return false; + if (!pte_dirty(pte)) + return false; + if (is_shadow_stack_mapping(vma->vm_flags)) + return false;
You had me up until this is_shadow_stack_mapping(). It wasn't mentioned at all in the changelog. Logically, I think it's trying to say that a shadow stack VMA never allows a FOLL_FORCE override. That makes some sense, but it's a pretty big point not to mention in the changelog.
quoted hunk ↗ jump to hunk
+ return true; } static struct page *follow_page_pte(struct vm_area_struct *vma,@@ -510,7 +518,7 @@ static struct page *follow_page_pte(struct vm_area_struct *vma, } if ((flags & FOLL_NUMA) && pte_protnone(pte)) goto no_page; - if ((flags & FOLL_WRITE) && !can_follow_write_pte(pte, flags)) { + if ((flags & FOLL_WRITE) && !can_follow_write_pte(pte, flags, vma)) { pte_unmap_unlock(ptep, ptl); return NULL; }