Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Dave Hansen <hidden>
Date: 2019-08-13 23:02:26
Also in:
linux-arch, linux-doc, linux-mm, lkml
+#if defined(CONFIG_X86_INTEL_SHADOW_STACK_USER)
+static inline pte_t pte_move_flags(pte_t pte, pteval_t from, pteval_t to)
+{
+ if (pte_flags(pte) & from)
+ pte = pte_set_flags(pte_clear_flags(pte, from), to);
+ return pte;Why is this conditional on the compile option and not a runtime check?
+}
+#else
+static inline pte_t pte_move_flags(pte_t pte, pteval_t from, pteval_t to)
+{
+ return pte;
+}
+#endifWhy do we need this function? It's not mentioned in the changelog or commented.
quoted hunk ↗ jump to hunk
static inline pte_t pte_mkclean(pte_t pte) { - return pte_clear_flags(pte, _PAGE_DIRTY); + return pte_clear_flags(pte, _PAGE_DIRTY_BITS); } static inline pte_t pte_mkold(pte_t pte)@@ -322,6 +336,7 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) { + pte = pte_move_flags(pte, _PAGE_DIRTY_HW, _PAGE_DIRTY_SW); return pte_clear_flags(pte, _PAGE_RW); }
Please comment what this is doing and why.
quoted hunk ↗ jump to hunk
@@ -332,9 +347,24 @@ static inline pte_t pte_mkexec(pte_t pte) static inline pte_t pte_mkdirty(pte_t pte) { + pteval_t dirty = (!IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER) || + pte_write(pte)) ? _PAGE_DIRTY_HW:_PAGE_DIRTY_SW;
This is *really* hard for me to read and parse. How about: pte_t dirty = _PAGE_DIRTY_HW; /* * When Shadow Stacks are enabled, read-only PTEs can * not have the hardware dirty bit set and must use * the software bit. */ if (IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER) && !pte_write(pte)) dirty = _PAGE_DIRTY_SW;
+ return pte_set_flags(pte, dirty | _PAGE_SOFT_DIRTY);
+}
+
+#ifdef CONFIG_ARCH_HAS_SHSTK
+static inline pte_t pte_mkdirty_shstk(pte_t pte)
+{
+ pte = pte_clear_flags(pte, _PAGE_DIRTY_SW);
return pte_set_flags(pte, _PAGE_DIRTY_HW | _PAGE_SOFT_DIRTY);
}Why does the _PAGE_DIRTY_SW *HAVE* to be cleared on shstk pages?
+static inline bool pte_dirty_hw(pte_t pte)
+{
+ return pte_flags(pte) & _PAGE_DIRTY_HW;
+}
+#endifWhy are these #ifdef'd?
quoted hunk ↗ jump to hunk
static inline pte_t pte_mkyoung(pte_t pte) { return pte_set_flags(pte, _PAGE_ACCESSED);@@ -342,6 +372,7 @@ static inline pte_t pte_mkyoung(pte_t pte) static inline pte_t pte_mkwrite(pte_t pte) { + pte = pte_move_flags(pte, _PAGE_DIRTY_SW, _PAGE_DIRTY_HW); return pte_set_flags(pte, _PAGE_RW); }
It also isn't clear to me why this *must* move bits here. Its doubly unclear why you would need to do this on systems when shadow stacks are compiled in but disabled. <snip> Same comments for pmds and puds.
quoted hunk ↗ jump to hunk
- /* mprotect needs to preserve PAT bits when updating vm_page_prot */ #define pgprot_modify pgprot_modify static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot)@@ -1178,6 +1254,19 @@ static inline int pmd_write(pmd_t pmd) return pmd_flags(pmd) & _PAGE_RW; } +static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot) +{ + pmdval_t val = pmd_val(pmd), oldval = val; + + val &= _HPAGE_CHG_MASK; + val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK; + val = flip_protnone_guard(oldval, val, PHYSICAL_PMD_PAGE_MASK); + if ((pmd_write(pmd) && !(pgprot_val(newprot) & _PAGE_RW))) + return pmd_move_flags(__pmd(val), _PAGE_DIRTY_HW, + _PAGE_DIRTY_SW); + return __pmd(val); +}
Why was this function moved? This makes it really hard to review what you changed I'm going to stop reading this code now. It needs a lot more care and feeding to make it reviewable. Please go back, double-check your changelogs and flesh them out, then please try to make the code more readable and understandable by commenting it. Please take all of the compile-time checks and ask yourself whether they need to be or *can* be runtime checks. Consider what the overhead is of non-shadowstack systems running shadowstack-required code. Please also reconcile the supervisor XSAVE portion of your patches with the ones that Fenghua has been sending around. I've given quite a bit of feedback to improve those. Please consolidate and agree on a common set of patches with him.