On Thu, Jun 7, 2018 at 11:23 AM Dave Hansen [off-list ref] wrote:

On 06/07/2018 09:24 AM, Andy Lutomirski wrote:

quoted

quoted

+static inline void ptep_set_wrprotect_flush(struct vm_area_struct *vma,
+                                           unsigned long addr, pte_t *ptep)
+{
+       bool rw;
+
+       rw = test_and_clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
+       if (IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER)) {
+               struct mm_struct *mm = vma->vm_mm;
+               pte_t pte;
+
+               if (rw && (atomic_read(&mm->mm_users) > 1))
+                       pte = ptep_clear_flush(vma, addr, ptep);

Why are you clearing the pte?

I think I insisted on this being in there.

First of all, we need to flush the TLB eventually because we need the
shadowstack PTE permissions to be in effect.

But, generally, we can't clear a dirty bit in a "live" PTE without
flushing.  The processor can keep writing until we flush, and even keep
setting it whenever _it_ allows a write, which it can do based on stale
TLB contents.  Practically, I think a walk to set the dirty bit is
mostly the same as a TLB miss, but that's certainly not guaranteed forever.

That's even ignoring all the fun errata we have.

Maybe make it a separate patch, then?  It seems like this patch is
doing two almost entirely separate things: adding some flushes and
adding the magic hwdirty -> swdirty transition.  As it stands, it's
quite difficult to read the patch and figure out what it does.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html