Re: [PATCH v2 12/39] x86/mm: Update ptep_set_wrprotect() and pmdp_set_wrprotect() for transition from _PAGE_DIRTY to _PAGE_COW

[Edgecombe, Rick P]

On Mon, 2022-10-03 at 16:25 -0700, Nadav Amit wrote:
> On Oct 3, 2022, at 4:20 PM, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
> 
> > On Oct 3, 2022, at 4:17 PM, Nadav Amit <nadav.amit@xxxxxxxxx>
> > wrote:
> > 
> > > On Oct 3, 2022, at 3:28 PM, Edgecombe, Rick P <
> > > rick.p.edgecombe@xxxxxxxxx> wrote:
> > > 
> > > > On Mon, 2022-10-03 at 11:11 -0700, Nadav Amit wrote:
> > > > > Did you have a look at ptep_set_access_flags() and friends
> > > > > and
> > > > > checked they
> > > > > do not need to be changed too? 
> > > > 
> > > > ptep_set_access_flags() doesn't actually set any additional
> > > > dirty bits
> > > > on x86, so I think it's ok.
> > > 
> > > Are you sure about that? (lost my confidence today so I am
> > > hesitant).
> > > 
> > > Looking on insert_pfn(), I see:
> > > 
> > >                        entry = maybe_mkwrite(pte_mkdirty(entry),
> > > vma);
> > >                        if (ptep_set_access_flags(vma, addr, pte,
> > > entry, 1)) ...
> > > 
> > > This appears to set the dirty bit while potentially leaving the
> > > write-bit
> > > clear. This is the scenario you want to avoid, no?
> > 
> > No. I am not paying attention. Ignore.
> 
> Sorry for the spam. Just this “dirty” argument is confusing. This
> indeed
> seems like a flow that can set the dirty bit. I think.

I think the HW dirty bit will not be set here. How it works is,
pte_mkdirty() will not actually set the HW dirty bit, but instead the
software COW bit. Here is the relevant snippet:

static inline pte_t pte_mkdirty(pte_t pte)
{
	pteval_t dirty = _PAGE_DIRTY;

	/* Avoid creating Dirty=1,Write=0 PTEs */
	if (cpu_feature_enabled(X86_FEATURE_SHSTK) && !pte_write(pte))
		dirty = _PAGE_COW;

	return pte_set_flags(pte, dirty | _PAGE_SOFT_DIRTY);
}

So for a !VM_WRITE vma, you end up with Write=0,Cow=1 PTE passed
into ptep_set_access_flags(). Does it make sense?