Re: [PATCH v5 10/39] x86/mm: Introduce _PAGE_COW

[David Hildenbrand]

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:
> > > +/*
> > > + * 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