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.
I will check, but..
> 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.
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.
--
Oscar Salvador
SUSE Labs
