Re: [PATCH] powerpc/pkeys: copy pkey-tracking-information at fork()

[Thiago Jung Bauermann <bauerman@linux.ibm.com>]

Ram Pai <linuxram@us.ibm.com> writes:

> At fork(), the pkey tracking information is not copied over
> to the mm_struct of the child. This can cause the child to erroneously
> allocate keys that were already allocated. Any allocated execute-only
> key is lost aswell.
>
> Add code; called by dup_mmap(), to copy the pkey state from
> parent to child explicitly.
>
> This problem was originally found by Dave Hansen on x86, which turns
> out to be a problem on powerpc aswell.
>
> Testing:
> 	Passes the new selftest added by Dave Hansen. I have temporarily
> 	captured that selftest at
> 	https://github.com/rampai/memorykeys/ -b memkey.v15-rc6
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/mmu_context.h |   15 +++++++++------
>  arch/powerpc/mm/pkeys.c                |    7 +++++++
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index 0381394..cb16146 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -217,12 +217,6 @@ static inline void enter_lazy_tlb(struct mm_struct *mm,
>  #endif
>  }
>
> -static inline int arch_dup_mmap(struct mm_struct *oldmm,
> -				struct mm_struct *mm)
> -{
> -	return 0;
> -}
> -
>  #ifndef CONFIG_PPC_BOOK3S_64
>  static inline void arch_exit_mmap(struct mm_struct *mm)
>  {
> @@ -247,6 +241,7 @@ static inline void arch_bprm_mm_init(struct mm_struct *mm,
>  #ifdef CONFIG_PPC_MEM_KEYS
>  bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
>  			       bool execute, bool foreign);
> +void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm);
>  #else /* CONFIG_PPC_MEM_KEYS */
>  static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
>  		bool write, bool execute, bool foreign)
> @@ -259,6 +254,7 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
>  #define thread_pkey_regs_save(thread)
>  #define thread_pkey_regs_restore(new_thread, old_thread)
>  #define thread_pkey_regs_init(thread)
> +#define arch_dup_pkeys(oldmm, mm)
>
>  static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>  {
> @@ -267,5 +263,12 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>
>  #endif /* CONFIG_PPC_MEM_KEYS */
>
> +static inline int arch_dup_mmap(struct mm_struct *oldmm,
> +				struct mm_struct *mm)
> +{
> +	arch_dup_pkeys(oldmm, mm);
> +	return 0;
> +}
> +
>  #endif /* __KERNEL__ */
>  #endif /* __ASM_POWERPC_MMU_CONTEXT_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index b271b28..5d65c47 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
>
>  	return pkey_access_permitted(vma_pkey(vma), write, execute);
>  }
> +
> +void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm)
> +{
> +	/* Duplicate the oldmm pkey state in mm: */
> +	mm_pkey_allocation_map(mm) = mm_pkey_allocation_map(oldmm);
> +	mm->context.execute_only_pkey = oldmm->context.execute_only_pkey;
> +}

This function is small and perhaps could have been a static inline in
<asm/mmu_context.h>, but arch_dup_mmap() doesn't seem to be in a hot
path so the segregation of implementation details to pkeys.c is nice
from an organization point of view.

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

--
Thiago Jung Bauermann
IBM Linux Technology Center