Re: [PATCH v2 6/9] mm: Make hugetlb mappings go through mm_get_unmapped_area_vmflags

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Jul 29, 2024 at 11:10:15AM GMT, Oscar Salvador wrote:
> Hugetlb mappings will no longer be special cased but rather go through
> the generic mm_get_unmapped_area_vmflags function.
> For that to happen, let us remove the .get_unmapped_area from
> hugetlbfs_file_operations struct, and hint __get_unmapped_area
> that it should not send hugetlb mappings through thp_get_unmapped_area_vmflags
> but through mm_get_unmapped_area_vmflags.
>
> Create also a function called hugetlb_mmap_check_and_align() where a
> couple of safety checks are being done and the addr is aligned to
> the huge page size.
> Otherwise we will have to do this in every single function, which
> duplicates quite a lot of code.
>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  fs/hugetlbfs/inode.c    | 22 ++++++++++++++--------
>  include/linux/hugetlb.h |  8 +++-----
>  mm/mmap.c               | 15 ++++++++++++++-
>  3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 9f6cff356796..5d47a2785a5d 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -258,15 +258,22 @@ generic_hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>  			pgoff, flags);
>  }
>
> -#ifndef HAVE_ARCH_HUGETLB_UNMAPPED_AREA
> -static unsigned long
> -hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> -			  unsigned long len, unsigned long pgoff,
> -			  unsigned long flags)
> +unsigned long
> +hugetlb_mmap_check_and_align(struct file *file, unsigned long addr,
> +			     unsigned long len, unsigned long flags)
>  {
> -	return generic_hugetlb_get_unmapped_area(file, addr, len, pgoff, flags);
> +	unsigned long addr0 = 0;
> +	struct hstate *h = hstate_file(file);
> +
> +	if (len & ~huge_page_mask(h))
> +		return -EINVAL;
> +	if ((flags & MAP_FIXED) && prepare_hugepage_range(file, addr, len))
> +		return -EINVAL;
> +	if (addr)
> +		addr0 = ALIGN(addr, huge_page_size(h));
> +
> +	return addr0;
>  }
> -#endif
>
>  /*
>   * Someone wants to read @bytes from a HWPOISON hugetlb @page from @offset.
> @@ -1300,7 +1307,6 @@ static const struct file_operations hugetlbfs_file_operations = {
>  	.read_iter		= hugetlbfs_read_iter,
>  	.mmap			= hugetlbfs_file_mmap,
>  	.fsync			= noop_fsync,
> -	.get_unmapped_area	= hugetlb_get_unmapped_area,

This is causing a NULL pointer deref error in the mm self-tests,
specifically hugepage-shm.

This is because in __get_unmapped_area(), you check to see if the file has
an f_ops->get_unampped_area() however ('wonderfully'...) the shm stuff
wraps it, so this will be shm_get_unmapped_area() which then accesses the
underlying hugetlb file and _unconditionally_ calls
f_op->get_unmapped_area(), which you just made NULL and... kaboom :)

You can't even add null check in to this wrapper as at this point
everything assumes that you _can_ get an unmapped area. So yeah, it's kinda
broken.

This makes me think the whole thing is super-delicate and you probably need
to rethink this approach carefully, or least _very carefully_ audit users
of this operation.

>  	.llseek			= default_llseek,
>  	.fallocate		= hugetlbfs_fallocate,
>  	.fop_flags		= FOP_HUGE_PAGES,
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 0ec14e5e0890..1413cdcfdb1a 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -549,11 +549,9 @@ static inline struct hstate *hstate_inode(struct inode *i)
>  }
>  #endif /* !CONFIG_HUGETLBFS */
>
> -#ifdef HAVE_ARCH_HUGETLB_UNMAPPED_AREA
> -unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> -					unsigned long len, unsigned long pgoff,
> -					unsigned long flags);
> -#endif /* HAVE_ARCH_HUGETLB_UNMAPPED_AREA */

By doing this you are causing an compilation error (at least on my compiler
with an x86-64 defconfig-based build):

arch/x86/mm/hugetlbpage.c:84:1: error: no previous prototype for
‘hugetlb_get_unmapped_area’ [-Werror=missing-prototypes]
   84 | hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~

So you need to rethink how you are doing this. I am not sure why we'd leave
these arch-specific bits around unless you're calling into them somehow
elsewhere? But this error suggests you aren't.

> +unsigned long
> +hugetlb_mmap_check_and_align(struct file *file, unsigned long addr,
> +			     unsigned long len, unsigned long flags);
>
>  unsigned long
>  generic_hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 7b623811d82a..f755d8a298c5 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -849,6 +849,7 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
>  				  unsigned long, unsigned long, unsigned long)
>  				  = NULL;
>
> +	bool is_hugetlb = false;
>  	unsigned long error = arch_mmap_check(addr, len, flags);
>  	if (error)
>  		return error;
> @@ -857,6 +858,9 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
>  	if (len > TASK_SIZE)
>  		return -ENOMEM;
>
> +	if (file && is_file_hugepages(file))
> +		is_hugetlb = true;
> +
>  	if (file) {
>  		if (file->f_op->get_unmapped_area)
>  			get_area = file->f_op->get_unmapped_area;
> @@ -874,11 +878,20 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
>
>  	if (get_area) {
>  		addr = get_area(file, addr, len, pgoff, flags);
> -	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> +	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !is_hugetlb) {
>  		/* Ensures that larger anonymous mappings are THP aligned. */
>  		addr = thp_get_unmapped_area_vmflags(file, addr, len,
>  						     pgoff, flags, vm_flags);
>  	} else {
> +		/*
> +		 * Consolidate hugepages checks in one place, and also align addr
> +		 * to hugepage size.
> +		 */
> +		if (is_hugetlb) {
> +			addr = hugetlb_mmap_check_and_align(file, addr, len, flags);
> +			if (IS_ERR_VALUE(addr))
> +				return addr;
> +		}
>  		addr = mm_get_unmapped_area_vmflags(current->mm, file, addr, len,
>  						    pgoff, flags, vm_flags);
>  	}
> --
> 2.45.2
>

Andrew - could we drop this from mm-unstable until Oscar can respin? As
it's causing the mm self tests to kernel panic and I'm using them a lot in
my testing of a new series thanks :>)