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 79A3AC48BF8 for ; Thu, 22 Feb 2024 08:37:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 087616B007E; Thu, 22 Feb 2024 03:37:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 037036B0080; Thu, 22 Feb 2024 03:37:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E413D6B0081; Thu, 22 Feb 2024 03:37:57 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id D2A626B007E for ; Thu, 22 Feb 2024 03:37:57 -0500 (EST) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 9F4FDA0D1A for ; Thu, 22 Feb 2024 08:37:57 +0000 (UTC) X-FDA: 81818786994.29.2A0E589 Received: from out-177.mta1.migadu.com (out-177.mta1.migadu.com [95.215.58.177]) by imf18.hostedemail.com (Postfix) with ESMTP id C33E81C000E for ; Thu, 22 Feb 2024 08:37:55 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Br3Ux179; spf=pass (imf18.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.177 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=1708591076; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc: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=rqsis7qCIfa4wD4HgfeAJauNuQNyODgqCcBJ3xvV6ng=; b=j+6usJC+pkWe2Z6sv6l/PMOaMJtoQsWI5D+XxEGWonwRcWZj2vYPxYMJzP8BUQMqDuFgbH qYING+nYj6Hza67viRkykRmP15Gchy2jcCVVGAh8eu/bkhJ13n48xvGGK7nbdOBSpVqzVL VIBE1TrcvyRRX+FSdqdwTwVlLbdpCnc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708591076; a=rsa-sha256; cv=none; b=CxeTsccuibN0qG0seR1RVlqmWwaUWlkZiidL6/J2VOBtdq/lUiWnpRqnsNwXe93cG1Bsmh 6ZwX2BeSVe6cb/4eCLRIvyLM3rJ9eV/AQ/xkz/fPxvaR9BwoGaXTVviYNseFe8Dkrg9jlx wPdpQNjwNXNqwJRqRoVRtaOMYlkVCOo= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Br3Ux179; spf=pass (imf18.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.177 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=1708591073; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rqsis7qCIfa4wD4HgfeAJauNuQNyODgqCcBJ3xvV6ng=; b=Br3Ux1791TakLd/z0p8E0Sf3d879NhVS0/I2uu+Fr8Ug5ZcT7ArqgrZPO1tMWC9nc7MQrz QljCIBIDbjCjyfoG+VBKHilsyGPazDapzS/gGbeeBZicHGWs4/PopowhRJsTKIxsNpHrUK wdc0V8CqMc3YAfbymN3cGKtAv4nKzsw= Date: Thu, 22 Feb 2024 16:37:44 +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 Cc: "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> <082fed0a-8489-37d1-f990-067976260659@linux.dev> 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: z4f1ptniau5igrryh9s3hsmeejb8hrme X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: C33E81C000E X-Rspam-User: X-HE-Tag: 1708591075-996546 X-HE-Meta: U2FsdGVkX1/+O+/WaMB/0DftSQ3KW8YGXd9cXPi0rT81MONJm3bKBqeQJrgzigfTetKVr6vFmm68TBCXfBA0us0C8/ja5gNoGYcnEAH27mW7dZt4QWauYusHrHVw0NZzkyOSmgGai04oRNPmE7q50foaDv4/KUyfPqJCFNkN2ba2PtMgPAdQyDPoqVqPMGnZd6c67+Gf5xlHTU73q5Cafr6+C3osU5K/6TJd8TSBEnUiHVxf8J/EsYBTmz2/rckkDSR5LayA4Z3a5bSg2o5ZKGNmbHn2r/oS2Y92iVTANN+YHrFyzQ7hQrjyDTsC39ylmfW3FGp5bdYP4A7dCl+ALbNi5TSnYBXiIOdgs/b0EOSLyFAg8ZeZAsDY/b9Uf/hs0ZsASMTdjYBVo+gSXk9M/E1N03vvzU9cRInntkFx7w5xfKefGyQ2C0KWhsLnyDYx8yB5/7L/r7i5Ypk0bWIC1nxPpWhZuPGyNpdjy+2apK10ySu1Zcwyjf/jsyykCVraTmc3OpuIRJ0ObwXqS0k2h2Ow23EGLpeGo3sQkZntfEK2x31jvmheavNs0yCKWgM1D+U34To5Mb3F8bKkPX+kFXR9TXNQWta3TTngWFTekxSh16IM7YappYmKkZeT3gyT21al2BAYPUKdQTLsQdss9thf0BoxPC4Vuorqy9t2FgDcjN8LemXit0c/eWPIF6HO3dqRVf17PM+SARZtnVvU5WoKR7jutGUG1yg/yKV1h3ARKbfFxv31XyxNpQJnWZHEt2HDIVky8LB/AT/oq0TgAEvjbrKMxkMsgynmmZZEfa5mQWB/a+1KsrPSNQMrglnLMCqysIODoLbW07IhRdwD/BV06yBPOIHD 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 16:31, Lorenzo Stoakes wrote: > On Thu, Feb 22, 2024 at 03:47:04PM +0800, Yajun Deng wrote: >> 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. > Absolutely not. I wrote the vma_modify() patch series explicitly to expose > _less_ not more. > >> 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: > As I said above, I really don't think this is a good idea, you're just > special casing one non-merge case but not any others + adding an > unnecessary branch. > >> 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) :) > I guess maybe again I've not been clear enough on this, so unless > compelling reasoning can otherwise be provided, I feel this check should > not be added _anywhere_. Okay, I got it. Thank you! > Therefore, NACK.