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 12552C48BF8 for ; Thu, 22 Feb 2024 07:47:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 847356B0072; Thu, 22 Feb 2024 02:47:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7F73F6B0074; Thu, 22 Feb 2024 02:47:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6E6126B0075; Thu, 22 Feb 2024 02:47:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 5B2346B0074 for ; Thu, 22 Feb 2024 02:47:20 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id E59CE160C67 for ; Thu, 22 Feb 2024 07:47:19 +0000 (UTC) X-FDA: 81818659398.18.BC87BC1 Received: from out-187.mta1.migadu.com (out-187.mta1.migadu.com [95.215.58.187]) by imf25.hostedemail.com (Postfix) with ESMTP id 9EA61A000A for ; Thu, 22 Feb 2024 07:47:16 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=JikvPYDB; spf=pass (imf25.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.187 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=1708588038; 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=P8ksOUceWP8fZMPWDbm1sqr9ILHsp0CLZKDJzGg7bMc=; b=HT3jCgVvOiQbOiYxkY/EA7TDrzJAuchGUkipqEGCSV0l0L+6wtef6NUiZncGGzmaEyp+Kn jbs321bC9jTWB/BRtPIPVv7V8A/jWUuLiaCG50JWzGU6mBTMUH1J+Ht+CENv/7Z6Bog/zo BH2G/XRRT3NG+p8dhCsJkNovGQjTQt8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708588038; a=rsa-sha256; cv=none; b=a6rm3pt9tfPH8PoE3a2aF6xz+N8FQ8ey4bEXKJJUApGeJF7prcpNeiz+alB3n8NH1M8f09 nJA2WhCp2Fk8hltooBVGAfNQRPmyzR19cetnx65/eXPCM3CUaQJvdfid3o4tYV8r/hTwwN GT1wLls07WKMQRJCvqKT2ORjHZYsS9A= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=JikvPYDB; spf=pass (imf25.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.187 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev; dmarc=pass (policy=none) header.from=linux.dev Message-ID: <082fed0a-8489-37d1-f990-067976260659@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1708588032; 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=P8ksOUceWP8fZMPWDbm1sqr9ILHsp0CLZKDJzGg7bMc=; b=JikvPYDB8OT6V9M2fxfrukoJSqhmrOclRBqT6Hs1bTz8HGpZvVFQ62eW5mGo5tA35KzqSx g16KjL7hkMVVfP4Tvdf6e1O8gb0x1OC01kVh4RY7jP/DfY+WNA51R5qdepptLnGq5m3n6a d8Zp9T4GBmWuTFC1ABdjDw2J+TOfVI8= Date: Thu, 22 Feb 2024 15:47:04 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v2] mm/mmap: return early if it can't merge in vma_merge() Content-Language: en-US To: Lorenzo Stoakes , "Liam R. Howlett" , akpm@linux-foundation.org, vbabka@suse.cz, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240221091453.1785076-1-yajun.deng@linux.dev> <20240221153827.wkmjnnwsf6lyxatc@revolver> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yajun Deng In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Stat-Signature: zbgjb9gfwxwxc3dbgxbtoy73pzmmxuh3 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 9EA61A000A X-Rspam-User: X-HE-Tag: 1708588036-187705 X-HE-Meta: U2FsdGVkX19uV8E61mg/IC/XykneyfoTIQ7mPm02Uo1ijAi/FwJ8jT6/l770EtCdvmOQwoEzhwvAujJiMllE/1lnSb2dJc3xD68lD1D/F/1ZJA+ckV6KsnS9O+nIFjsevQVcPuiy6C3tOP8yIp168/yX9C5rmtWsSiuJF/xZgDBsMD7FvZZt7fR46MqsoeMCsPUwpHfF14apAw4K8fXc94aeSO3G/atFzTFeCCRrQaYWj0DqwWzNuSDvN7v25fkASRU50+UK8UyJHGSnWqBx9xVFjilWmdHiHDlhiJEkX/nGCzuWSqA3RkcVtryp5YhbivCMgNREFqDlv3cgJQ0zBNNUGAgJ8uNAW7vt056paYgjVUPWNksWsRoD+VrUYgROGSmtiVyvSyRf4tS3nrD1KkhxaqNnIErHmkEHx6YDxzr1teVYSUVkpyFF1tQvLHhYw091gso3JXejR2UQTJRgZPG090cAbOfId359Ho298bNqHBagjZCrKkVj7O5iUpNhzk/2nuupBdzvOHpIpJSPy5DDYMmJ59hwsd20ZCer5zVyTN2GL+F7QCJpm8k39CJwD1P88qAyVu0jVB9Ms/09qhFRXI/g8iTdNW+9qq+17rQDgB37lZVyQRuBtCOIsAHojcQRz+CbPJqOEFFZt53dYVJBBrocHMUztrtjBTdSRtFqE31tk2gB8at2Mtg/5yn6RsXW4vBSVDhN7HYLYwZRxzIRG2xBtgCfDgkgfIccaUuWdnR+jyzKRUJiC4rX8LByLCBXQfSaf8zNBxfcy9gEoNUf4KKpb6sO2RbHtl1ZtLgC7WW6CEcmqw2gYMWC24p0H3+xQBvkz7TGgsHsCeBbkQH4BmQka5wg 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/2/22 04:41, Lorenzo Stoakes wrote: > On Wed, Feb 21, 2024 at 10:38:27AM -0500, Liam R. Howlett wrote: >> * Yajun Deng [240221 04:15]: >>> In most cases, the range of the area is valid. But in do_mprotect_pkey(), >>> the minimum value of end and vma->vm_end is passed to mprotect_fixup(). >>> This will lead to the end is less than the end of prev. >>> >>> In this case, the curr will be NULL, but the next will be equal to the >>> prev. So it will attempt to merge before, the vm_pgoff check will cause >>> this case to fail. >>> >>> To avoid the process described above and reduce unnecessary operations. >>> Add a check to immediately return NULL if the end is less than the end of >>> prev. >> If it's only one caller, could we stop that caller instead of checking >> an almost never case for all callers? Would this better fit in >> vma_modify()? Although that's not just for this caller at this point. >> Maybe there isn't a good place? > I definitely agree with Liam that this should not be in vma_merge(), as > it's not going to be relevant to _most_ callers. > > I am not sure vma_modify() is much better, this would be the only early > exit check in that function and makes what is very simple and > straightforward now more confusing. There are two paths that will cause this case. One is in mprotect_fixup(), the other is in madvise_update_vma(). One way is to separate out the split_vma() from vma_modify(). And create a new helper function. We can call it directly at source, but we need to do this in both paths.  It's more complicated. The other way is to add a check in vma_modify(). Like the following: diff --git a/mm/mmap.c b/mm/mmap.c index 0fccd23f056e..f93f1d3939f2 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2431,11 +2431,15 @@ struct vm_area_struct *vma_modify(struct vma_iterator *vmi,         pgoff_t pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);         struct vm_area_struct *merged; +       if (prev && end < prev->vm_end) +               goto cannot_merge; +         merged = vma_merge(vmi, prev, vma, start, end, vm_flags,                            pgoff, policy, uffd_ctx, anon_name);         if (merged)                 return merged; +cannot_merge:         if (vma->vm_start < start) {                 int err = split_vma(vmi, vma, start, 1); > And I think this is the crux of it - it's confusing that we special case > this one particular non-merge scenario, but no others (all of which we then > deem ok to be caught by the usual rules). > > I think it's simpler (and more efficient) to just keep things the way they > are. > >> Or are there other reasons this may happen and is better done in this >> function? >> >> Often, this is called the "punch a hole" scenario; where an operation >> creates two entries from the old data and either leaves an empty space >> or fills the space with a new VMA. >> >>> Signed-off-by: Yajun Deng >>> --- >>> v2: remove the case label. >>> v1: https://lore.kernel.org/all/20240218085028.3294332-1-yajun.deng@linux.dev/ >>> --- >>> mm/mmap.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/mm/mmap.c b/mm/mmap.c >>> index 0fccd23f056e..7668854d2246 100644 >>> --- a/mm/mmap.c >>> +++ b/mm/mmap.c >>> @@ -890,6 +890,9 @@ static struct vm_area_struct >>> if (vm_flags & VM_SPECIAL) >>> return NULL; >>> >>> + if (prev && end < prev->vm_end) >>> + return NULL; >>> + >>> /* 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 >>> > So overall I don't think this check makes much sense anywhere. > > I think a better solution would be to prevent it happening _at source_ if > you can find a logical way of doing so. > > I do plan to do some cleanup passes over this stuff once again so maybe I > can figure something out that better handles non-merge scenarios. > > This is a great find though overall even if a patch doesn't make sense > Yajun, thanks for this, it's really made me think about this case (+ others > like it) :)