Re: [RFC PATCH v9 16/27] mm: Update can_follow_write_pte() for Shadow Stack

Dave Hansen
2020-02-27T00:34:55Z

> +inline bool pte_exclusive(pte_t pte, struct vm_area_struct *vma)
> +{
> + if (vma->vm_flags & VM_SHSTK)
> + return pte_dirty_hw(pte);
> + else
> + return pte_dirty(pte);
> +}

I'm not really getting the naming. What is exclusive?

> diff --git a/mm/gup.c b/mm/gup.c
> index 7646bf993b25..d1dbfbde8443 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -164,10 +164,12 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address,
> * FOLL_FORCE can write to even unwritable pte's, but only
> * after we've gone through a COW cycle and they are dirty.
> */
> -static inline bool can_follow_write_pte(pte_t pte, unsigned int flags)
> +static inline bool can_follow_write(pte_t pte, unsigned int flags,
> + struct vm_area_struct *vma)

Having two identically named functions in two files in the same
subsystem seems like a recipe for confusion when I grep or cscope for
things. It hardly seems worth the 4 characters of space savings IMNHO.

> {
> return pte_write(pte) ||
> - ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_dirty(pte));
> + ((flags & FOLL_FORCE) && (flags & FOLL_COW) &&
> + pte_exclusive(pte, vma));
> }

FWIW, this is the hunk that fixed DirtyCOW.

The least this deserves is acknowledgement of that in the changelog and
a missive about how you're sure you didn't just introduce
ShadowDirtyCOW. Don't bother. I already registered the domain. ;)