linux-kernel - Re: [PATCH v3 4/5] mm: remove unnecessary reset state logic on merge new 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: <21e7bed9-b705-4fb1-84e6-9fab2e66dbeb@suse.cz>
Date: Fri, 25 Oct 2024 19:35:22 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: "Liam R . Howlett" <Liam.Howlett@...cle.com>, Jann Horn
 <jannh@...gle.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Linus Torvalds <torvalds@...ux-foundation.org>, Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH v3 4/5] mm: remove unnecessary reset state logic on merge
 new VMA

On 10/25/24 14:26, Lorenzo Stoakes wrote:
> The only place where this was used was in mmap_region(), which we have now
> adjusted to not require this to be performed (we reset ourselves in
> effect).
> 
> It also created a dangerous assumption that VMG state could be safely
> reused after a merge, at which point it may have been mutated in unexpected
> ways, leading to subtle bugs.
> 
> Note that it was discovered by Wei Yang that there was also an error in
> this code - we are comparing vmg->vma with prev after setting it to
> NULL.
> 
> This however had no impact, as we previously reset VMA iterator state
> before attempting merge again, but it was useless effort.
> 
> In any case, this patch removes all of the logic so also eliminates this
> wasted effort.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

Yeah glad to be rid of this.

Reviewed-by: Vlastimil Babka <vbabka@...e.cz>

> ---
>  mm/vma.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/mm/vma.c b/mm/vma.c
> index b91c947babd6..7c690be67910 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -963,7 +963,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>  	struct vm_area_struct *next = vmg->next;
>  	unsigned long start = vmg->start;
>  	unsigned long end = vmg->end;
> -	pgoff_t pgoff = vmg->pgoff;
>  	pgoff_t pglen = PHYS_PFN(end - start);
>  	bool can_merge_left, can_merge_right;
>  	bool just_expand = vmg->merge_flags & VMG_FLAG_JUST_EXPAND;
> @@ -1020,16 +1019,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>  		return vmg->vma;
>  	}
>  
> -	/* If expansion failed, reset state. Allows us to retry merge later. */
> -	if (!just_expand) {
> -		vmg->vma = NULL;
> -		vmg->start = start;
> -		vmg->end = end;
> -		vmg->pgoff = pgoff;
> -		if (vmg->vma == prev)
> -			vma_iter_set(vmg->vmi, start);
> -	}
> -
>  	return NULL;
>  }
>