linux-kernel - Re: [PATCH 10/10] mm/mremap: simplify vma expansion again

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]

Date:   Wed, 15 Mar 2023 22:20:46 +0000
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        maple-tree@...ts.infradead.org
Subject: Re: [PATCH 10/10] mm/mremap: simplify vma expansion again

On Thu, Mar 09, 2023 at 12:12:58PM +0100, Vlastimil Babka wrote:
> This effectively reverts d014cd7c1c35 ("mm, mremap: fix mremap()
> expanding for vma's with vm_ops->close()"). After the recent changes,
> vma_merge() is able to handle the expansion properly even when the vma
> being expanded has a vm_ops->close operation, so we don't need to
> special case it anymore.
>
> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
> ---
>  mm/mremap.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 411a85682b58..65f5b545601e 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1040,23 +1040,11 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>  			 * vma (expand operation itself) and possibly also with
>  			 * the next vma if it becomes adjacent to the expanded
>  			 * vma and  otherwise compatible.
> -			 *
> -			 * However, vma_merge() can currently fail due to
> -			 * is_mergeable_vma() check for vm_ops->close (see the
> -			 * comment there). Yet this should not prevent vma
> -			 * expanding, so perform a simple expand for such vma.
> -			 * Ideally the check for close op should be only done
> -			 * when a vma would be actually removed due to a merge.
>  			 */
> -			if (!vma->vm_ops || !vma->vm_ops->close) {
> -				vma = vma_merge(&vmi, mm, vma, extension_start,
> -					extension_end, vma->vm_flags, vma->anon_vma,
> -					vma->vm_file, extension_pgoff, vma_policy(vma),
> -					vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> -			} else if (vma_expand(&vmi, vma, vma->vm_start,
> -					addr + new_len, vma->vm_pgoff, NULL)) {
> -				vma = NULL;
> -			}
> +			vma = vma_merge(&vmi, mm, vma, extension_start,
> +				extension_end, vma->vm_flags, vma->anon_vma,
> +				vma->vm_file, extension_pgoff, vma_policy(vma),
> +				vma->vm_userfaultfd_ctx, anon_vma_name(vma));
>  			if (!vma) {
>  				vm_unacct_memory(pages);
>  				ret = -ENOMEM;
> --
> 2.39.2
>

Good to eliminate this edge case! Do we have a self-test for this case to assert
that the issue is fixed by this? I guess a little tricky due to the need for the
the owning VMA to have ->close() specified.

In any case, the changes you have made in the previous patch should ensure the
edge case is no longer required, hence:-

Reviewed-by: Lorenzo Stoakes <lstoakes@...il.com>