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@xxxxxxx>
> ---
> 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 :>)
