linux-kernel - Re: [PATCH v3 10/16] mm/mmap: Reposition vma iterator in mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <76625760-1fc6-4c81-9df9-ee47697995ff@lucifer.local>
Date: Fri, 5 Jul 2024 21:18:16 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Lorenzo Stoakes <lstoakes@...il.com>,
        Matthew Wilcox <willy@...radead.org>, sidhartha.kumar@...cle.com,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Bert Karwatzki <spasswolf@....de>, Jiri Olsa <olsajiri@...il.com>,
        linux-kernel@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH v3 10/16] mm/mmap: Reposition vma iterator in
 mmap_region()

On Thu, Jul 04, 2024 at 02:27:12PM GMT, Liam R. Howlett wrote:
> From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
>
> Instead of moving (or leaving) the vma iterator pointing at the previous
> vma, leave it pointing at the insert location.  Pointing the vma
> iterator at the insert location allows for a cleaner walk of the vma
> tree for MAP_FIXED and the no expansion cases.

I mean, it's funny I litearlly just asked why it was being left pointing at
prev and I guess this answers that basically :)

>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> ---
>  mm/mmap.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f5b33de4e717..ecf55d32e804 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2963,11 +2963,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		vms_complete_munmap_vmas(&vms, &mas_detach);
>  		next = vms.next;
>  		prev = vms.prev;
> -		vma_prev(&vmi);
>  		vma = NULL;
>  	} else {
>  		next = vma_next(&vmi);
>  		prev = vma_prev(&vmi);
> +		if (prev)
> +			vma_iter_next_range(&vmi);
>  	}
>
>  	/*
> @@ -2980,11 +2981,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		vm_flags |= VM_ACCOUNT;
>  	}
>
> -	if (vm_flags & VM_SPECIAL) {
> -		if (prev)
> -			vma_iter_next_range(&vmi);
> +	if (vm_flags & VM_SPECIAL)
>  		goto cannot_expand;
> -	}
>
>  	/* Attempt to expand an old mapping */
>  	/* Check next */
> @@ -3005,19 +3003,21 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		merge_start = prev->vm_start;
>  		vma = prev;
>  		vm_pgoff = prev->vm_pgoff;
> -	} else if (prev) {
> -		vma_iter_next_range(&vmi);
> +		vma_prev(&vmi);

Why not vma_iter_prev_range()?

>  	}
>
> -	/* Actually expand, if possible */
> -	if (vma &&
> -	    !vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
> -		khugepaged_enter_vma(vma, vm_flags);
> -		goto expanded;
> +	if (vma) {
> +		/* Actually expand, if possible */
> +		if (!vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
> +			khugepaged_enter_vma(vma, vm_flags);
> +			goto expanded;
> +		}
> +
> +		/* If the expand fails, then reposition the vma iterator */
> +		if (unlikely(vma == prev))
> +			vma_iter_set(&vmi, addr);
>  	}
>
> -	if (vma == prev)
> -		vma_iter_set(&vmi, addr);
>  cannot_expand:
>
>  	/*
> --
> 2.43.0
>
Looks good to me,

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>