Re: [PATCH] mm: allow checking length for hugetlb mapping in mmap()

[John Hubbard <jhubbard@nvidia.com>]

On 3/28/20 8:08 PM, Li Xinhai wrote:
> In current code, the vma related call of hugetlb mapping, except mmap,
> are all consider not correctly aligned length as invalid parameter,
> including mprotect,munmap, mlock, etc., by checking through
> hugetlb_vm_op_split. So, user will see failure, after successfully call
> mmap, although using same length parameter to other mapping syscall.
> 
> It is desirable for all hugetlb mapping calls have consistent behavior,
> without mmap as exception(which round up length to align underlying
> hugepage size). In current Documentation/admin-guide/mm/hugetlbpage.rst,
> the description is:
> "
> Syscalls that operate on memory backed by hugetlb pages only have their
> lengths aligned to the native page size of the processor; they will
> normally fail with errno set to EINVAL or exclude hugetlb pages that
> extend beyond the length if not hugepage aligned. For example, munmap(2)
> will fail if memory is backed by a hugetlb page and the length is smaller
> than the hugepage size.
> "
> which express the consistent behavior.


Missing here is a description of what the patch actually does...

> 
> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
> changes:
> 0. patch which introduce new flag for mmap()
>     The new flag should be avoided.
>     https://lore.kernel.org/linux-mm/1585313944-8627-1-git-send-email-lixinhai.lxh@gmail.com/
> 
>   mm/mmap.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d681a20..b2aa102 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1560,20 +1560,12 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
>   		file = fget(fd);
>   		if (!file)
>   			return -EBADF;
> -		if (is_file_hugepages(file))
> -			len = ALIGN(len, huge_page_size(hstate_file(file)));


...and it looks like this is simply removing the forced alignment. And not adding
any error case for non-aligned cases. So now I'm not immediately sure what happens if a
non-aligned address is passed in.

I would have expected to see either error checking or an ALIGN call here, but now both
are gone, so I'm lost and confused. :)


thanks,
-- 
John Hubbard
NVIDIA

>   		retval = -EINVAL;
>   		if (unlikely(flags & MAP_HUGETLB && !is_file_hugepages(file)))
>   			goto out_fput;
>   	} else if (flags & MAP_HUGETLB) {
>   		struct user_struct *user = NULL;
> -		struct hstate *hs;
>   
> -		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
> -		if (!hs)
> -			return -EINVAL;
> -
> -		len = ALIGN(len, huge_page_size(hs));
>   		/*
>   		 * VM_NORESERVE is used because the reservations will be
>   		 * taken when vm_ops->mmap() is called
>