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

[Lorenzo Stoakes]

On Wed, Jul 31, 2024 at 05:08:24PM GMT, Oscar Salvador wrote:
> On Wed, Jul 31, 2024 at 12:02:47PM +0100, Lorenzo Stoakes wrote:
> > On Mon, Jul 29, 2024 at 11:10:15AM GMT, Oscar Salvador wrote:
> > >   * 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.
>
> Thanks for reporting this Lorenzo, highly appreciated.
>

No problem :)

> I will check, but..

Yeah this is obviously the priority.

>
> > 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,
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Something is off here.
>
> git grep hugetlb_get_unmapped_area
>
> returns nothing.

Yeah this is at commit aee8efc95fc2 ("mm: make hugetlb mappings go through
mm_get_unmapped_area_vmflags").

If you:

git checkout aee8efc95fc2
git grep hugetlb_get_unmapped_area

You'll see it.

I'm guessing you remove this in future commits, but the kernel must be able
to build at every revision so we can bisect (I found this issue through a
bisect and had to fix this up to check).

A trivial fix is just to provide the prototype immediately prior to the
function decl, however the more correct solution is probably to do the
removals at the same time.

> After this, arch/x86/mm/hugetlbpage.c should only contain:
>
>   #ifdef CONFIG_X86_64
>   bool __init arch_hugetlb_valid_size(unsigned long size)
>   {
>           if (size == PMD_SIZE)
>                   return true;
>           else if (size == PUD_SIZE && boot_cpu_has(X86_FEATURE_GBPAGES))
>                   return true;
>           else
>                   return false;
>   }
>
>   #ifdef CONFIG_CONTIG_ALLOC
>   static __init int gigantic_pages_init(void)
>   {
>           /* With compaction or CMA we can allocate gigantic pages at runtime */
>           if (boot_cpu_has(X86_FEATURE_GBPAGES))
>                   hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
>           return 0;
>   }
>   arch_initcall(gigantic_pages_init);
>   #endif
>   #endif
>
> so what is going here?
> Maybe the series was not properly applied to mm-unstable?
>
> I will have a look.

I see this being removed in commit 631dc86d2f95 ("mm: drop
hugetlb_get_unmapped_area{_*} functions") which comes after aee8efc95fc2,
so basically I think you should squash the two... or at least find a way to
adjust them so this error doesn't arise.

This bit is just a bit of a slightly nitty cleanup to make sure things
build at every commit, the first issue is the really key one, just needs
some tweaking to deal with the frankly bloody horrible SHM stuff... Do not
blame you for missing that one!

Cheers :)

>
>
> --
> Oscar Salvador
> SUSE Labs