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

[Borislav Petkov <bp@alien8.de>]

On Fri, Feb 10, 2023 at 05:00:05PM +0000, Edgecombe, Rick P wrote:
> 	/*
> 	 * 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?

I think so.

1. If you have a single pte_mkdirty() and not also a __ helper, then
   there's less confusion for callers as to which interface they should be
   using

2. The not centralized part is a single conditional so it's not like
   you're saving on gazillion code lines

So I'd prefer that.

If we end up needing this in more places then we can carve it out into
a proper helper which is not in a header file such that anyone can use
it but move the whole functionality into cet.c or so where we can
control its visibility to the rest of the kernel.

I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette