Re: [PATCH] mm,madvise,hugetlb: check for 0-length range after end address adjustment

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 31 Jan 2025 15:37:49 +0100 Ricardo Cañuelo Navarro <rcn@igalia.com> wrote:

> Add a sanity check to madvise_dontneed_free() to address a corner case
> in madvise where a race condition causes the current vma being processed
> to be backed by a different page size.
> 
> During a madvise(MADV_DONTNEED) call on a memory region registered with
> a userfaultfd, there's a period of time where the process mm lock is
> temporarily released in order to send a UFFD_EVENT_REMOVE and let
> userspace handle the event. During this time, the vma covering the
> current address range may change due to an explicit mmap done
> concurrently by another thread.
> 
> If, after that change, the memory region, which was originally backed by
> 4KB pages, is now backed by hugepages, the end address is rounded down
> to a hugepage boundary to avoid data loss (see "Fixes" below). This
> rounding may cause the end address to be truncated to the same address
> as the start.
> 
> Make this corner case follow the same semantics as in other similar
> cases where the requested region has zero length (ie. return 0).
> 
> This will make madvise_walk_vmas() continue to the next vma in the
> range (this time holding the process mm lock) which, due to the prev
> pointer becoming stale because of the vma change, will be the same
> hugepage-backed vma that was just checked before. The next time
> madvise_dontneed_free() runs for this vma, if the start address isn't
> aligned to a hugepage boundary, it'll return -EINVAL, which is also in
> line with the madvise api.
> 
> >From userspace perspective, madvise() will return EINVAL because the
> start address isn't aligned according to the new vma alignment
> requirements (hugepage), even though it was correctly page-aligned when
> the call was issued.
> 
> ...
>
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -933,7 +933,9 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
>  			 */
>  			end = vma->vm_end;
>  		}
> -		VM_WARN_ON(start >= end);
> +		if (start == end)
> +			return 0;
> +		VM_WARN_ON(start > end);
>  	}

Perhaps add a comment telling the user how this situation can come about?