Re: [RFC 1/4] mm: Introduce lazy exec permission setting on a page

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Feb 15, 2019 at 01:41:16PM +0530, Anshuman Khandual wrote:
> On 02/14/2019 02:36 PM, Mike Rapoport wrote:
> > On Wed, Feb 13, 2019 at 07:23:18PM +0530, Anshuman Khandual wrote:
> >> On 02/13/2019 06:47 PM, Matthew Wilcox wrote:
> >>> On Wed, Feb 13, 2019 at 01:36:28PM +0530, Anshuman Khandual wrote:
> >>>> +#ifdef CONFIG_ARCH_SUPPORTS_LAZY_EXEC
> >>>> +static inline pte_t maybe_mkexec(pte_t entry, struct vm_area_struct *vma)
> >>>> +{
> >>>> +	if (unlikely(vma->vm_flags & VM_EXEC))
> >>>> +		return pte_mkexec(entry);
> >>>> +	return entry;
> >>>> +}
> >>>> +#else
> >>>> +static inline pte_t maybe_mkexec(pte_t entry, struct vm_area_struct *vma)
> >>>> +{
> >>>> +	return entry;
> >>>> +}
> >>>> +#endif
> >>>
> >>>> +++ b/mm/memory.c
> >>>> @@ -2218,6 +2218,8 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
> >>>>  	flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
> >>>>  	entry = pte_mkyoung(vmf->orig_pte);
> >>>>  	entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> >>>> +	if (vmf->flags & FAULT_FLAG_INSTRUCTION)
> >>>> +		entry = maybe_mkexec(entry, vma);
> >>>
> >>> I don't understand this bit.  We have a fault based on an instruction
> >>> fetch.  But we're only going to _maybe_ set the exec bit?  Why not call
> >>> pte_mkexec() unconditionally?
> >>
> >> Because the arch might not have subscribed to this in which case the fall
> >> back function does nothing and return the same entry. But in case this is
> >> enabled it also checks for VMA exec flag (VM_EXEC) before calling into
> >> pte_mkexec() something similar to existing maybe_mkwrite().
> > 
> > Than why not pass vmf->flags to maybe_mkexec() so that only arches
> > subscribed to this will have the check for 'flags & FAULT_FLAG_INSTRUCTION' ?
> 
> Right it can help remove couple of instructions from un-subscribing archs. 

If the arch does not enable CONFIG_ARCH_SUPPORTS_LAZY_EXEC, wouldn't the
compiler eliminate the FAULT_FLAG_INSTRUCTION check anyway? The current
maybe_mkexec() proposal here looks slightly nicer as it matches the
maybe_mkwrite() prototype.

-- 
Catalin