Re: [PATCH v3 02/12] mm: Introduce arch_get_unmapped_area_vmflags()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 12/03/2024 à 23:28, Rick Edgecombe a écrit :
> When memory is being placed, mmap() will take care to respect the guard
> gaps of certain types of memory (VM_SHADOWSTACK, VM_GROWSUP and
> VM_GROWSDOWN). In order to ensure guard gaps between mappings, mmap()
> needs to consider two things:
>   1. That the new mapping isn’t placed in an any existing mappings guard
>      gaps.
>   2. That the new mapping isn’t placed such that any existing mappings
>      are not in *its* guard gaps.
> 
> The long standing behavior of mmap() is to ensure 1, but not take any care
> around 2. So for example, if there is a PAGE_SIZE free area, and a
> mmap() with a PAGE_SIZE size, and a type that has a guard gap is being
> placed, mmap() may place the shadow stack in the PAGE_SIZE free area. Then
> the mapping that is supposed to have a guard gap will not have a gap to
> the adjacent VMA.
> 
> In order to take the start gap into account, the maple tree search needs
> to know the size of start gap the new mapping will need. The call chain
> from do_mmap() to the actual maple tree search looks like this:
> 
> do_mmap(size, vm_flags, map_flags, ..)
> 	mm/mmap.c:get_unmapped_area(size, map_flags, ...)
> 		arch_get_unmapped_area(size, map_flags, ...)
> 			vm_unmapped_area(struct vm_unmapped_area_info)
> 
> One option would be to add another MAP_ flag to mean a one page start gap
> (as is for shadow stack), but this consumes a flag unnecessarily. Another
> option could be to simply increase the size passed in do_mmap() by the
> start gap size, and adjust after the fact, but this will interfere with
> the alignment requirements passed in struct vm_unmapped_area_info, and
> unknown to mmap.c. Instead, introduce variants of
> arch_get_unmapped_area/_topdown() that take vm_flags. In future changes,
> these variants can be used in mmap.c:get_unmapped_area() to allow the
> vm_flags to be passed through to vm_unmapped_area(), while preserving the
> normal arch_get_unmapped_area/_topdown() for the existing callers.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>   include/linux/sched/mm.h | 17 +++++++++++++++++
>   mm/mmap.c                | 28 ++++++++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index cde946e926d8..7b44441865c5 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -191,6 +191,23 @@ unsigned long mm_get_unmapped_area(struct mm_struct *mm, struct file *filp,
>   				   unsigned long addr, unsigned long len,
>   				   unsigned long pgoff, unsigned long flags);
>   
> +extern unsigned long
> +arch_get_unmapped_area_vmflags(struct file *filp, unsigned long addr,
> +			       unsigned long len, unsigned long pgoff,
> +			       unsigned long flags, vm_flags_t vm_flags);
> +extern unsigned long
> +arch_get_unmapped_area_topdown_vmflags(struct file *filp, unsigned long addr,
> +				       unsigned long len, unsigned long pgoff,
> +				       unsigned long flags, vm_flags_t);

Please, no new "extern", that's pointless for function prototypes and 
outdate coding style. Checkpatch --strict is likely protesting about it.

> +
> +unsigned long mm_get_unmapped_area_vmflags(struct mm_struct *mm,
> +					   struct file *filp,
> +					   unsigned long addr,
> +					   unsigned long len,
> +					   unsigned long pgoff,
> +					   unsigned long flags,
> +					   vm_flags_t vm_flags);
> +
>   unsigned long
>   generic_get_unmapped_area(struct file *filp, unsigned long addr,
>   			  unsigned long len, unsigned long pgoff,
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 39e9a3ae3ca5..e23ce8ca24c9 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1810,6 +1810,34 @@ arch_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
>   }
>   #endif
>   
> +#ifndef HAVE_ARCH_UNMAPPED_AREA_VMFLAGS
> +extern unsigned long
> +arch_get_unmapped_area_vmflags(struct file *filp, unsigned long addr, unsigned long len,
> +			       unsigned long pgoff, unsigned long flags, vm_flags_t vm_flags)
> +{
> +	return arch_get_unmapped_area(filp, addr, len, pgoff, flags);
> +}

Same, what is the point with that "extern" keyword in function definition ?

> +
> +extern unsigned long
> +arch_get_unmapped_area_topdown_vmflags(struct file *filp, unsigned long addr,
> +				       unsigned long len, unsigned long pgoff,
> +				       unsigned long flags, vm_flags_t vm_flags)
> +{
> +	return arch_get_unmapped_area_topdown(filp, addr, len, pgoff, flags);
> +}
> +#endif
> +
> +unsigned long mm_get_unmapped_area_vmflags(struct mm_struct *mm, struct file *filp,
> +					   unsigned long addr, unsigned long len,
> +					   unsigned long pgoff, unsigned long flags,
> +					   vm_flags_t vm_flags)
> +{
> +	if (test_bit(MMF_TOPDOWN, &mm->flags))
> +		return arch_get_unmapped_area_topdown_vmflags(filp, addr, len, pgoff,
> +							      flags, vm_flags);
> +	return arch_get_unmapped_area_vmflags(filp, addr, len, pgoff, flags, vm_flags);
> +}
> +
>   unsigned long
>   get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
>   		unsigned long pgoff, unsigned long flags)