Re: [PATCH v5 11/39] x86/mm: Update pte_modify for _PAGE_COW

[Edgecombe, Rick P]

On Fri, 2023-02-10 at 14:57 +0100, Borislav Petkov wrote:
> @@ -749,14 +740,8 @@ static inline pte_t pte_modify(pte_t pte,
> pgprot_t newprot)
>  
>         pte_result = __pte(val);
>  
> -       /*
> -        * Dirty bit is not preserved above so it can be done
> -        * in a special way for the shadow stack case, where it
> -        * may need to set _PAGE_COW. __pte_mkdirty() will do this in
> -        * the case of shadow stack.
> -        */
>         if (pte_dirty(pte))
> -               pte_result = __pte_mkdirty(pte_result, false);
> +               pte_result = pte_set_flags(pte_result, _PAGE_DIRTY);
>  
>         return pte_result;
>  }
> 

Oh, I see what you are seeing now. Did you notice that  the
__pte_mkdirty() logic got expanded in "x86/mm: Start actually marking
_PAGE_COW"? So if we don't put that logic in a usable helper, it ends
up open coded with pte_modify() looking something 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 the NX bit (if present), and add the NX portion of
	 * the newprot (if present):
	 */
	val &= (_PAGE_CHG_MASK & ~_PAGE_DIRTY);
	val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK;
	val = flip_protnone_guard(oldval, val, PTE_PFN_MASK);

	pte_result = __pte(val);

	/*
	 * Dirty bit is not preserved above so it can be done
	 * in a special way for the shadow stack case, where it
	 * may need to set _PAGE_SAVED_DIRTY. __pte_mkdirty() will do
	 * this in the case of shadow stack.
	 */
	if (oldval & _PAGE_DIRTY)
		if (cpu_feature_enabled(X86_FEATURE_USER_SHSTK) &&
		    !pte_write(pte_result))
			pte_set_flags(pte_result, _PAGE_SAVED_DIRTY);
		else
			pte_set_flags(pte_result, _PAGE_DIRTY);
	}

	return pte_result;
}

So the later logic of doing the _PAGE_SAVED_DIRTY (_PAGE_COW) part is
not centralized. It's ok?