linux-kernel - Re: [PATCH] mm/mmap: Add case 9 in vma

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: <a5cd692e-34e3-4bc1-a8fa-f6bb56f04e8a@lucifer.local>
Date: Sun, 18 Feb 2024 23:03:58 +0000
From: Lorenzo Stoakes <lstoakes@...il.com>
To: Yajun Deng <yajun.deng@...ux.dev>
Cc: akpm@...ux-foundation.org, Liam.Howlett@...cle.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, vbabka@...e.cz
Subject: Re: [PATCH] mm/mmap: Add case 9 in vma_merge()

On Sun, Feb 18, 2024 at 04:50:28PM +0800, Yajun Deng wrote:
> If the prev vma exists and the end is less than the end of prev, we
> can return NULL immediately. This reduces unnecessary operations.
>
> Signed-off-by: Yajun Deng <yajun.deng@...ux.dev>

Adding Vlastimil, while get_maintainers.pl might not show it very clearly,
myself, Vlastimil and Liam often work with vma_merge() so it's handy to cc
us on these if you can!

> ---
>  mm/mmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 8f176027583c..b738849321c0 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -827,7 +827,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
>   *
>   *     ****             ****                   ****
>   *    PPPPPPNNNNNN    PPPPPPNNNNNN       PPPPPPCCCCCC
> - *    cannot merge    might become       might become
> + *    cannot merge 9  might become       might become

While I welcome your interest here :) I am not a fan of the 'case' approach
to this function as-is and plan to heavily refactor this when I get a chance.

But at any rate, an early-exit situation is not a merge case, merge cases
describe cases where we _can_ merge, so we can drop this case 9 stuff (this
is not your fault, it's understandable why you would label this, this
function is just generally unclear).

>   *                    PPNNNNNNNNNN       PPPPPPPPPPCC
>   *    mmap, brk or    case 4 below       case 5 below
>   *    mremap move:
> @@ -890,6 +890,9 @@ static struct vm_area_struct
>  	if (vm_flags & VM_SPECIAL)
>  		return NULL;
>
> +	if (prev && end < prev->vm_end) /* case 9 */
> +		return NULL;
> +

I need to get back into vma_merge() head space, but I don't actually think
a caller that's behaving correctly should ever do this. I know the ASCII
diagram above lists it as a thing that can happen, but I think we
implicitly avoid this from the way we invoke callers. Either prev == vma as
per vma_merge_extend(), or the loops that invoke vma_merge_new_vma()
wouldn't permit this to occur.

Let me look into it more deeply + reply again a bit later, I mean we could
perhaps do with asserting this somehow, but I don't think it's useful to do
an early exit for something that ostensibly _shouldn't_ happen.

>  	/* Does the input range span an existing VMA? (cases 5 - 8) */
>  	curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end);
>
> --
> 2.25.1
>