Re: [PATCH v4 48/49] mm/mmap: Remove __vma_adjust()

[Vlastimil Babka]

On 1/20/23 17:26, Liam R. Howlett wrote:
> From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> 
> Inline the work of __vma_adjust() into vma_merge().  This reduces code
> size and has the added benefits of the comments for the cases being
> located with the code.
> 
> Change the comments referencing vma_adjust() accordingly.
> 
> Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>

...

> @@ -1054,32 +945,85 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  					     vm_userfaultfd_ctx, anon_name)) {
>  		merge_next = true;
>  	}
> +
> +	remove = remove2 = adjust = NULL;
>  	/* Can we merge both the predecessor and the successor? */
>  	if (merge_prev && merge_next &&
> -			is_mergeable_anon_vma(prev->anon_vma,
> -				next->anon_vma, NULL)) {	 /* cases 1, 6 */
> -		err = __vma_adjust(vmi, prev, prev->vm_start,
> -					next->vm_end, prev->vm_pgoff, prev);
> -		res = prev;
> -	} else if (merge_prev) {			/* cases 2, 5, 7 */
> -		err = __vma_adjust(vmi, prev, prev->vm_start,
> -					end, prev->vm_pgoff, prev);
> -		res = prev;
> +	    is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL)) {
> +		remove = mid;				/* case 1 */
> +		vma_end = next->vm_end;
> +		err = dup_anon_vma(res, remove);
> +		if (mid != next) {			/* case 6 */
> +			remove2 = next;
> +			if (!remove->anon_vma)
> +				err = dup_anon_vma(res, remove2);
> +		}
> +	} else if (merge_prev) {
> +		err = 0;				/* case 2 */
> +		if (mid && end > mid->vm_start) {
> +			err = dup_anon_vma(res, mid);
> +			if (end == mid->vm_end) {	/* case 7 */
> +				remove = mid;
> +			} else {			/* case 5 */
> +				adjust = mid;
> +				adj_next = (end - mid->vm_start);
> +			}
> +		}
>  	} else if (merge_next) {
> -		if (prev && addr < prev->vm_end)	/* case 4 */
> -			err = __vma_adjust(vmi, prev, prev->vm_start,
> -					addr, prev->vm_pgoff, next);
> -		else					/* cases 3, 8 */
> -			err = __vma_adjust(vmi, mid, addr, next->vm_end,
> -					next->vm_pgoff - pglen, next);
>  		res = next;
> +		if (prev && addr < prev->vm_end) {	/* case 4 */
> +			vma_end = addr;
> +			adjust = mid;
> +			adj_next = -(vma->vm_end - addr);
> +			err = dup_anon_vma(res, adjust);

I think this one is wrong, and should be fixed as below. I'm not
exactly sure about user visible effects, but shouldn't matter if
we fix before rc1? I guess what can happen is we end up with pages
becoming part of 'prev' that have anon_vma originally from 'mid'
which is not connected to 'prev', so eventually some rmap operation
will fail to do the right thing etc. Or 'mid' is unmapped, its
anon_vma freed and we have a use-after free. Probably rare to happen,
but nasty enough.

----8<----