From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C80B8C46CD2 for ; Wed, 24 Jan 2024 03:45:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D6DDA6B0088; Tue, 23 Jan 2024 22:45:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CCF318D0001; Tue, 23 Jan 2024 22:45:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B218C6B008A; Tue, 23 Jan 2024 22:45:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 9D38A6B0088 for ; Tue, 23 Jan 2024 22:45:55 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 63CF1402BC for ; Wed, 24 Jan 2024 03:45:55 +0000 (UTC) X-FDA: 81712815870.14.E01D8B8 Received: from out-182.mta0.migadu.com (out-182.mta0.migadu.com [91.218.175.182]) by imf29.hostedemail.com (Postfix) with ESMTP id 9B454120027 for ; Wed, 24 Jan 2024 03:45:52 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=SFSoh2Rh; spf=pass (imf29.hostedemail.com: domain of yajun.deng@linux.dev designates 91.218.175.182 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706067953; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5E1jGheheG3ZM1Tb7z2Aj6BvnsCBIRhjOg5678MeZtQ=; b=6rYsi3P9KVXtCc7AwdCgoSf+IUedA8YYuEIhjRATzP6jHPxCs2y8iE+e48UVd2ecbEqQRG hw53DdG9gEXUt9k3uF7BQMklhW2OTcHQoz/RSXiV3aAYUTMfZ7+oY5KxAw7V6R3dtwI3V3 OGkdlk2qHTdqyNOl/z/sK9VeHGW8HXE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706067953; a=rsa-sha256; cv=none; b=05CrDxEk2la3CFNyWBTO4VMDnK4XnhIXDxH4cMvbkhTYqdPX6RMBRa7amVL+BLSpS/77Cp KIb6XQCD6CMAYbUc1UAT5nhv4zuPreqpvRFP41SJ2gcTxQzCPaLrV254dytg8ZgTh+5Z/b 1PZb3fspk71PuW3Z79fEo/wr7Ysyw9k= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=SFSoh2Rh; spf=pass (imf29.hostedemail.com: domain of yajun.deng@linux.dev designates 91.218.175.182 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev; dmarc=pass (policy=none) header.from=linux.dev Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1706067950; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5E1jGheheG3ZM1Tb7z2Aj6BvnsCBIRhjOg5678MeZtQ=; b=SFSoh2Rh1bwPkTGQpSPACKNWAoGiyh9o4PmVEuTZplErIdaRcb2jTyC1cdK0jC5JRln8I/ pfnKwffntvVHRVXZ99LIosZTo9iR8yWt1900s9l4f3NSEG/k72mXSwyLI3BngoIDGNXO/s peHmb+rXON+w8KCUhSP4IzoiU+1fg6M= Date: Wed, 24 Jan 2024 11:45:41 +0800 MIME-Version: 1.0 Subject: Re: [PATCH] mm/mmap: simplify vma_merge() Content-Language: en-US To: "Liam R. Howlett" , akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Lorenzo Stoakes , Alexander Viro , Christian Brauner , Vlastimil Babka , Matthew Wilcox , Suren Baghdasaryan References: <20240118082312.2801992-1-yajun.deng@linux.dev> <20240123160855.k7fhmlb7cfhdel5t@revolver> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yajun Deng In-Reply-To: <20240123160855.k7fhmlb7cfhdel5t@revolver> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Stat-Signature: bpdjx6rit3xugfonhyqmenifyd7jydc3 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 9B454120027 X-Rspam-User: X-HE-Tag: 1706067952-994888 X-HE-Meta: U2FsdGVkX1/4BJ+jnV3RlH17rrog67ORAqcpvrzKIOSdFVEMW57xArTBFpMXe/gQBGP6NyvmYnGeeyJALDUOFE/n3ivTVhtnBOmd6tz8LZrEGdzsAfEnBTM7DQ3Y1QC4JiqoTNlIIz+AIW0Z8bnDryRD3tu8S4ti7oz7oUpobiBG3wuUWdKs106Qor9S+42Py4S9eUIK06bOdulYKWgc43ip895zViEGZfCJTCMFQzJyhbpMFwlDYYzrzkpX1gyfUztCcNvBdPT5Vu3pO/EvcaqhUEtAKEgUM5Zs+wVjuevxazNY7rjUWIgaIaDY2O6utKFmDSW59qns8hGN7evk81uY3ALK499E43H69ht+Y37k1Jh4TeQpBM1YbDjCBqlBwVpMqZvBAbeKKsVIyAHGe1el2Q5VD7DWM/9407vAYUxDKUpBBld5JNUJg5nXjy1yvQTMcl3YjW1lNMOQZlOe4n0yHrvYIDo06qCJzkMEE6a6H+ifxKRPW9n/JvBMp0mIRta4GY9dFcF/gPNj4KygmzeVe/5Ld7hhuT4xYN3oGmsS6tIikwLPXk3szlOviisSI/roEjYMrQt+AWcyJd3NAZbhYlx/rlB1ArvjolV6LrWA6eUXOD8N0iH5V1bpSqSOp+d2A5A+YorPOrAaabDq/T3Wl34a6bvTX3QBJd6bH+nLYGtpUAQ1GhIxiIwUQ6rriJgTm1MzqKWdIv8gLfI45rPS1wlNKogit9o5L+HyUACdOx2kOROC2vx0edKYZ4XLm7lAcaLMe9nuCxgQ2O+8lR27xbtdayadJL7FbG3veMq+FBy101gG7XVFdgYqGD8kPFWcXV8VKAjEHtaxnwheyHcWX5ausU+/ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2024/1/24 00:08, Liam R. Howlett wrote: > Adding to the Cc list, because it's vma_merge(). > > * Yajun Deng [240118 03:23]: >> These vma_merge() callers will pass mm, anon_vma and file, they all from >> vma. There is no need to pass three parameters at the same time. >> >> We will find the current vma in vma_merge(). > It sounds like you are adding a search for current to vma_merger(), but > you are removing that part in your patch, so it's odd to say this here. > Okay. >> If we pass the original vma >> to vma_merge(), the current vma is actually the original vma or NULL. > What do you mean original vma? The source of the anon_vma, vm_mm, etc? > If so, the 'original' vma could be prev (shifting boundaries in case 4 > and 5 in the comments). I think "vma that was the source of the > arguments" would be more clear than "original vma". > Okay. >> So we didn't need to find the current vma with find_vma_intersection(). >> >> Pass vma to vma_merge(), and add a check to make sure the current vma >> is an existing vma. > How could it not be an existing vma? It is dereferenced, so it exists. > Do you mean a vma in the vma tree? It means the current vma is NULL or not. > I think this is all to say that we can pass through the vma to figure > out if curr == NULL, or if it's vma directly. > Okay. >> Signed-off-by: Yajun Deng >> --- >> mm/mmap.c | 37 +++++++++++++++++-------------------- >> 1 file changed, 17 insertions(+), 20 deletions(-) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index 49d25172eac8..7e00ae4f39e3 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -860,14 +860,16 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags, >> * area is returned, or the function will return NULL >> */ >> static struct vm_area_struct >> -*vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, >> - struct vm_area_struct *prev, unsigned long addr, unsigned long end, >> - unsigned long vm_flags, struct anon_vma *anon_vma, struct file *file, >> - pgoff_t pgoff, struct mempolicy *policy, >> +*vma_merge(struct vma_iterator *vmi, struct vm_area_struct *prev, >> + struct vm_area_struct *curr, unsigned long addr, unsigned long end, >> + unsigned long vm_flags, pgoff_t pgoff, struct mempolicy *policy, >> struct vm_userfaultfd_ctx vm_userfaultfd_ctx, >> struct anon_vma_name *anon_name) >> { >> - struct vm_area_struct *curr, *next, *res; >> + struct mm_struct *mm = curr->vm_mm; >> + struct anon_vma *anon_vma = curr->anon_vma; >> + struct file *file = curr->vm_file; >> + struct vm_area_struct *next = NULL, *res; >> struct vm_area_struct *vma, *adjust, *remove, *remove2; >> struct vm_area_struct *anon_dup = NULL; >> struct vma_prepare vp; >> @@ -889,13 +891,12 @@ static struct vm_area_struct >> return NULL; >> >> /* Does the input range span an existing VMA? (cases 5 - 8) */ >> - curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end); >> + if (prev == curr || addr != curr->vm_start || end > curr->vm_end) >> + curr = NULL; > It would be nice to have comments about what cases this logic covers, > because reverse engineering it is a pain. And we have to do it every > time a change occurs in the function, even when we are the ones who > wrote the statement. I think we can all agree that this function is > painful, but it's improving and thanks for joining. Okay. >> >> if (!curr || /* cases 1 - 4 */ >> end == curr->vm_end) /* cases 6 - 8, adjacent VMA */ >> - next = vma_lookup(mm, end); >> - else >> - next = NULL; /* case 5 */ >> + next = vma_lookup(mm, end); /* NULL case 5 */ > Ah, maybe put the comment about case 5 being null on a different line. > I thought you were saying the vma_lookup() will return NULL, not that it > was initialised as NULL above. Change the wording to something like > "case 5 set to NULL above" or "case 5 remains NULL". > Okay. >> >> if (prev) { >> vma_start = prev->vm_start; >> @@ -919,7 +920,6 @@ static struct vm_area_struct >> >> /* Verify some invariant that must be enforced by the caller. */ >> VM_WARN_ON(prev && addr <= prev->vm_start); >> - VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end)); > Why did you drop this? I understand you moved basically all of it to an > if statement above, but it's still true, right? Considering the > trickiness of the function I'd like to keep it if there's no one who > feels strongly about it. I don't think we need this. We move this to the front of the function, addr, end and curr won't be changed until then. >> VM_WARN_ON(addr >= end); >> > ... > > To increase the chances of actually finding an issue, I would suggest > splitting this into two patches: > > 1. Just passing through vma. > 2. The logic changes to remove that find_vma_intersection() call. Okay. > By the way, what are the performance benefits to this change? It's not > without its own risks - this function has caused subtle bugs that > persisted for several releases in the past and it'd be nice to know what > we are gaining for the risk. No, I just found out that the current vma is the source vma. So we don't need to find the current vma with find_vma_intersection(). I think we can add some case about vma_merge() to the LTP project. It currently has 5 test cases about vma, but it doesn't seem to detect the risk of vma_merge(). Link: http://linux-test-project.github.io/ > > Thanks, > Liam