On 1/30/22 13:18, Rick Edgecombe wrote:

From: Yu-cheng Yu <redacted>

The read-only and Dirty PTE has been used to indicate copy-on-write pages.

Nit: This is another opportunity to use consistent terminology
     for these Write=0,Dirty=1 PTEs.

However, newer x86 processors also regard a read-only and Dirty PTE as a
shadow stack page.  In order to separate the two, the software-defined
_PAGE_COW is created to replace _PAGE_DIRTY for the copy-on-write case, and
pte_*() are updated.

The tense here is weird.  "_PAGE_COW is created" is present tense, but
it refers to something that happened earlier in the series.

Pte_modify() changes a PTE to 'newprot', but it doesn't use the pte_*().

I'm not seeing a clear problem statement in there.  It looks something
like this to me:

	pte_modify() takes a "raw" pgprot_t which was not necessarily
	created with any of the existing PTE bit helpers.  That means
	that it can return a pte_t with Write=0,Dirty=1: a shadow stack
	PTE when it did not intend to create one.

But, this kinda looks like a hack to me.

It all boils down to _PAGE_CHG_MASK.  If pte_modify() can change the
bit's value, it is not included in _PAGE_CHG_MASK.  But, pte_modify()
*CAN* change the _PAGE_DIRTY value now.

Another way of saying it is that _PAGE_DIRTY is now a permission bit
(part-time, at least).

Maybe something like this?  We can take _PAGE_DIRTY out of
_PAGE_CHG_MASK, then the p*_modify() functions look like this:

static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
{
        pteval_t val = pte_val(pte), oldval = val;
+	pte_t pte_result;

        /* Chop off any bits that might change with 'newprot':  */
        val &= _PAGE_CHG_MASK;
        val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK;
        val = flip_protnone_guard(oldval, val, PTE_PFN_MASK);

+       pte_result = __pte(val);
+
+	if (pte_dirty(oldval))
+		pte_result = pte_mkdirty(pte_result));
+	
+	return pte_result;
}


This:

1. Makes logical sense: the dirty bit *IS* special in that it has to be
   logically preserved across permission changes.
2. Would work with or without shadow stacks.  That exact code would even
   work on a non-shadow-stack kernel
3. Doesn't introduce *any* new shadow-stack conditional code; the one
   already hidden in pte_mkdirty() is sufficient.
4. Avoids silly things like setting a bit and then immediately clearing
   it in a "fixup".
5. Removes the opaque "fixup" abstraction function.

That's way better if I do say so myself.