On 23.01.23 21:56, Edgecombe, Rick P wrote:

Trying to answer both questions to this patch on this one.

On Mon, 2023-01-23 at 10:28 +0100, David Hildenbrand wrote:

quoted

quoted

+/*
+ * Normally COW memory can result in Dirty=1,Write=0 PTEs. But in
the case
+ * of X86_FEATURE_USER_SHSTK, the software COW bit is used, since
the
+ * Dirty=1,Write=0 will result in the memory being treated as
shadow stack
+ * by the HW. So when creating COW memory, a software bit is used
+ * _PAGE_BIT_COW. The following functions pte_mkcow() and
pte_clear_cow()
+ * take a PTE marked conventionally COW (Dirty=1) and transition
it to the
+ * shadow stack compatible version of COW (Cow=1).
+ */

TBH, I find that all highly confusing.

Dirty=1,Write=0 does not indicate a COW page reliably. You could
have
both, false negatives and false positives.

False negative: fork() on a clean anon page.

False positives: wrpotect() of a dirty anon page.


I wonder if it really has to be that complicated: what you really
want
to achieve is to disallow "Dirty=1,Write=0" if it's not a shadow
stack
page, correct?

The other thing is to save that the PTE is/was Dirty=1 somewhere (for
non-shadow stack memory). A slightly different but related thing. But
losing that information would would introduce differences for
pte_dirty() between when shadow stack was enabled or not. GUP/COW
doesn't need this anymore but there are lots of other places it gets
checked.

Perhaps following your GUP changes, _PAGE_COW is just now the wrong
name for it. _PAGE_SAVED_DIRTY maybe?

It goes into the direction of my other proposal/idea, yes. Not sure if 
_PAGE_SAVED_DIRTY would currently mimic what's happening here ... 
_PAGE_COW is certainly wrong and misleading.

-- 
Thanks,

David / dhildenb