Re: [PATCH] mm/mmap: Align the length parameter of munmap with hugepage size

[Lorenzo Stoakes]

On Wed, Jul 10, 2024 at 01:45:58PM GMT, Haoran Jang wrote:
> From: Haoran Jiang <jianghaoran@xxxxxxxxxx>
>
> munmap hugepge mappings, if the length of the range to munmap
> is not aligned with hugepage size,munmap will fail.
> In the hugetlb_vm_op_split function, an error will be returned
> if startaddr+len is not hugepage size aligned.
>
> before this patch:
> in "tools/testing/selftests/mm/hugepage-mremap.c"
> modify DEFAULT_LENGTH_MB to 3M,compile and run,
> the following error message is displayed
>
> -------------------------
> running ./hugepage-mremap
> -------------------------
> TAP version 13
> 1..1
> Map haddr: Returned address is 0x7eaa40000000
> Map daddr: Returned address is 0x7daa40000000
> Map vaddr: Returned address is 0x7faa40000000
> Address returned by mmap() = 0x7cb34b000000
> Mremap: Returned address is 0x7faa40000000
> First hex is 0
> First hex is 3020100
> Bail out! mremap: Expected failure, but call succeeded
>
> Signed-off-by: Haoran Jiang <jianghaoran@xxxxxxxxxx>
> ---
>  mm/mmap.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 83b4682ec85c..0b3a60bf9b6f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2733,7 +2733,15 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
>  	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
>  		return -EINVAL;
>
> -	end = start + PAGE_ALIGN(len);
> +	vma = find_vma(mm, start);
> +	if (!vma) {
> +		if (unlock)
> +			mmap_write_unlock(mm);
> +		return 0;
> +	}

I really don't like this, firstly we're duplicating the VMA lookup (we
vma_find() below), and we fail to use the iterator here, and also we are
duplicating the unlock logic.

Also the semantics seem wrong, we are looking for a VMA that ends at or
after start, so you're just checking to see if start is past the last VMA
in the mm aren't you?

This doesn't seem to be accomplishing anything too useful, unless I'm
missing something?

> +
> +	end = start + ALIGN(len, vma_kernel_pagesize(vma));
> +

This seems to be the 'action' part of the change, but I'm concerned this is
completely broken, because you're using the result of find_vma() passed
into vma_kernel_pagesize() which could find a VMA _after_ the input range,
and end up unmapping a far wider range...

I'm also wondering if we should be doing some hugetlb-specific logic here,
or whether that belongs elsewhere?

Liam can chime in on that.

>  	if (end == start)
>  		return -EINVAL;
>
> --
> 2.43.0
>