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 6D1F9C47DD9 for ; Thu, 22 Feb 2024 22:07:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D69276B007E; Thu, 22 Feb 2024 17:07:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D19AE6B0081; Thu, 22 Feb 2024 17:07:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C093E6B0082; Thu, 22 Feb 2024 17:07:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id B29EE6B007E for ; Thu, 22 Feb 2024 17:07:10 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 4DABA140E79 for ; Thu, 22 Feb 2024 22:07:10 +0000 (UTC) X-FDA: 81820826220.14.D62E50C Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf11.hostedemail.com (Postfix) with ESMTP id 4E58D4001B for ; Thu, 22 Feb 2024 22:07:08 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cujybF+p; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf11.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708639628; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=2xO2+giGNpQMfCaj0W7NZUwdaHnujksHyJgZqAdjT78=; b=l9yRqi0sWLv0WMaxQmBdzQIBQ7LN3lwsGJLubzVO6UTc7CK/VGjYNbG4mRLJt5pbPNX50T OtaFOZSVHIwtNo9Lun2DCxM5ldGFlFpr6NHbVU//OSTkOOqeJ0lNnq41I5piEJAbw2AO3n PfPzzKvZDOIbUMeQl1J08gQMwhk9uIc= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cujybF+p; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf11.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708639628; a=rsa-sha256; cv=none; b=gWTxym0OG5gSMT9fPbj2EChFn/IhfXbWdxHwFyX3lXpd0Fbn1xGJBAD0MpXbpXCRJr4yAf 4ZXIINrJQNMfDIgexjZDDUXlVsTRfIE8TG65ooe7r1O06QcPI5O0wpMr3EU6O9WlLUZwQl QG5ycsnREcKA/I/yhI8Edhsw3rVrBKU= Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-4129015c31eso1644135e9.2 for ; Thu, 22 Feb 2024 14:07:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708639627; x=1709244427; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=2xO2+giGNpQMfCaj0W7NZUwdaHnujksHyJgZqAdjT78=; b=cujybF+p/FmbcN7NYYeqp8ndnxVANnCLvoLeNQBbCr2egQVNHRrzmYw7WAUaJdu2Kc JWBEzADPWI89qTQo2fk/UTxge/JO9xdC+Sq6+Y7bIFvSxDL3fNdPNWSqodctZ0LsLHz7 kgP9pMczX4P7Dg8WgBYAFG3meJE4rtc4eSHMWyYgmPBtCZKfFe+71K4vglsPg9aYDUma VY5Q6Xuz7YLGO3GKXIaRv/9BXigryeVy/2BKcUWBzy19yHEhtOmpZeFipjX8F4LCqMlG u2jL1XjVI5r4hYEoAdfge5MWaX9DqGJbsFafRP5Vj86laNM5729y5ZB2pQpqvOVpZQ/a WKew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708639627; x=1709244427; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=2xO2+giGNpQMfCaj0W7NZUwdaHnujksHyJgZqAdjT78=; b=svyMrRKbEH+Hoybmh7ExQqaDsBsabrwykycD9tVP3JqP/0DKbH0HkWzMGfbEGJasvf pFRTCbzHxxWpcJLMs5XdqPjko5DOpbtzYEK1JYROIO3acaSzq2BLh2m7gdlNG+QPl96N rUCe7sD2xOF4OBd4N22kaMEv3qcijbX3PsLGWi76TG627yuOB8xlqt6FgJzH2gt1FHEG neSPEr7FkX+5zrnBYzSgNYJ4yNeKVDnsCNSYrphrEOB/DUnzIwFHA1/xX+o9LvmkX02a FGTllkVzoUQKsoKBmsJsV4HxRIWjh4hdoHYaJ+3AS+QV5iUEc0+AjUoq8v7uDk6DA8Vj CdkA== X-Forwarded-Encrypted: i=1; AJvYcCUv0fRkIYHWvc8GZSfaWJTfyrn0aJNTN4BqvvOKqzXlDllpCK6phxGRUvIIgi+uzx/sHkpbeY0n2zAaz1tUKBl9z4o= X-Gm-Message-State: AOJu0YzlV7PNOQcN6yCkrXcoIbpCruATsXwZys72wSz03/FsWN5Ze650 CTIJTCBa792bLG5FSvGuMQD/QSku0H5QzY/q8wt3trePhl5SzcRr X-Google-Smtp-Source: AGHT+IGT5fmok8cVA7Uk0NaxAKCJUmuvQkQ5FUjnauyj6OzPFKueFjkypLIrNylHVuxYqbopszOwOA== X-Received: by 2002:adf:ec81:0:b0:33d:73e4:9c79 with SMTP id z1-20020adfec81000000b0033d73e49c79mr228529wrn.59.1708639626512; Thu, 22 Feb 2024 14:07:06 -0800 (PST) Received: from localhost (host86-164-109-77.range86-164.btcentralplus.com. [86.164.109.77]) by smtp.gmail.com with ESMTPSA id h14-20020adfa4ce000000b0033d56aa4f45sm317637wrb.112.2024.02.22.14.07.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Feb 2024 14:07:05 -0800 (PST) Date: Thu, 22 Feb 2024 22:04:51 +0000 From: Lorenzo Stoakes To: Vlastimil Babka Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Michal Hocko , "Liam R . Howlett" , stable@vger.kernel.org Subject: Re: [PATCH v2] mm, mmap: fix vma_merge() case 7 with vma_ops->close Message-ID: <8dc5c86e-f74d-4547-99c9-3bd4a0d92cde@lucifer.local> References: <20240222215930.14637-2-vbabka@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240222215930.14637-2-vbabka@suse.cz> X-Rspamd-Queue-Id: 4E58D4001B X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: ufhbq5ahiqjthannjinqomj5bz334bz6 X-HE-Tag: 1708639628-105253 X-HE-Meta: U2FsdGVkX18MoO6v80f48HQ0e/QImxiKzsNFBeLAZ0tIbtFCgKcJeNbido7NhQgtxjyl6GXJA1SyQ7unt+1la/9XwS/hiyHI6Vhg/DL7IvhZm30rkIkcsxbZPJ7bebwn+LntzwYP9CKBBdsbOHhanDZ4Hzu8qz7jU3djzYxlNizLqc4fFTr3sO3ecE1Y2Vxsq8ISCTy0QlBMfy6c3V5PO+TCvCUfvDnreHF3cdjGFdRA9Cn1wVKH+Zfq4ueSxkRu/YZ+HTdei9hiAHqk2xEIRn4xEJfjGPlV0mnj6TALJA0XUA/JK0Jo7kxziXrWabb9hwrBx0QezuWCKifCjfnvIWcbvQwfbBGuBhiJUGKwvBM29xMLUpXe2fIuXMk0Kc9a1HY4FwI3hH+zKeGnp3MDBSCQ5weT99i7/SuqSC4+Xf5kh0e+WFzCIXN8Rod2B7R7C/70gZsg12OGh23kedVzKWZs6eEfPfzUWWw7JZBTEz+jRVXO+pu9GImlWDG9RrV9WeV5SnQ59YbYpg0MF2RujQpQtEL1aI6n1IBe91rOzPeAGxS6Q45k/l8Wv9jYNi/FawGwp8PJUHjDEyCvLKbNCnbjJnKl0ujPcwKNiI1qpEuvuA78YufbHwDSzBwCVuPV/QlT5Myv7I6a+0hEEGVdWDJMAz14svFUVKpytX+By3L169761hJhIxb4in+UlPy5NcAu0/oKCr8CfNCzYB8tJuc0w3gXf75U4MiYnp/AOioVxEG0b8rqHnViGiuwfvEuRX3U6eyZo1ijt6IsdbjUxzXerbsR+BvU0t9848LutRkxGfLzpZT3+/AujkgF5I7SdyRcEGAO/x0f86HtR0wnlIty6iZXELL1RQQelH/DSOG2NvITQU7HnT23SkxT5a4svjF2ETPiv8lXYyjl0mgCwwP12xzb1POgard7MwfQlyu2Tqe3IPORX3hPRM7q/w5j+YEousBPsZPSCABzpTA mOB4Udgb /zkB8YPVeVpUpuYpGyZOnOo5qAu04tCyqAuvCTROD05DHsGGY7wIE9eVaINHVc8G0HGZaZd/u+C+4w7yK8pVBwOan3j+YEvZn4Cu94MGNnv9+b6eW9jANJAxGAdE4YsIihk1tdPRF7YAtAm3+MGdz2EUSAkJr8QJGtrvRdJBTCOZlosXepqj80+AVnAHveDJXJ7GVYaCy8F4lB1dqTEDhPu+mRcutGu14XARv0M7K42JJ2gXwIjlWq87oYGtXdiFqbUgAOZbGZcTcr3gnD/aanxH96P2xRFnuE3ESbMvq0SVVrP5fRaIKpoSF8ELiwwLkwtIpT+4Bt73Br6F5d5dedbE3uNab4o0OuP/ZSuwXNySoBbHYXJC4xwICRxotvtJETj+vCtLTq6P60dq/vGBqG6EeZu3rpOng+uDcMFM2Dg+0/pp6TaoslvHVfZfMBMH6aJw1LeSEK7/WoS03pJO6ZEEBmF/uki3HZOGsjKpE/uQXiM8/ycvZ6fMM14/Y1hAPJFp56jSY2TfAlImM0CIVpyiYK3kK/fQyzy1czkM1FlAI4VtK9OPvmP2ZMinVT8l5g2k0WfLlYiZlGd51uT2W+2jG1FJbrgz0GzhJ 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 Thu, Feb 22, 2024 at 10:59:31PM +0100, Vlastimil Babka wrote: > When debugging issues with a workload using SysV shmem, Michal Hocko has > come up with a reproducer that shows how a series of mprotect() > operations can result in an elevated shm_nattch and thus leak of the > resource. > > The problem is caused by wrong assumptions in vma_merge() commit > 714965ca8252 ("mm/mmap: start distinguishing if vma can be removed in > mergeability test"). The shmem vmas have a vma_ops->close callback > that decrements shm_nattch, and we remove the vma without calling it. > > vma_merge() has thus historically avoided merging vma's with > vma_ops->close and commit 714965ca8252 was supposed to keep it that way. > It relaxed the checks for vma_ops->close in can_vma_merge_after() > assuming that it is never called on a vma that would be a candidate for > removal. However, the vma_merge() code does also use the result of this > check in the decision to remove a different vma in the merge case 7. > > A robust solution would be to refactor vma_merge() code in a way that > the vma_ops->close check is only done for vma's that are actually going > to be removed, and not as part of the preliminary checks. That would > both solve the existing bug, and also allow additional merges that the > checks currently prevent unnecessarily in some cases. > > However to fix the existing bug first with a minimized risk, and for > easier stable backports, this patch only adds a vma_ops->close check to > the buggy case 7 specifically. All other cases of vma removal are > covered by the can_vma_merge_before() check that includes the test for > vma_ops->close. > > The reproducer code, adapted from Michal Hocko's code: > > int main(int argc, char *argv[]) { > int segment_id; > size_t segment_size = 20 * PAGE_SIZE; > char * sh_mem; > struct shmid_ds shmid_ds; > > key_t key = 0x1234; > segment_id = shmget(key, segment_size, > IPC_CREAT | IPC_EXCL | S_IRUSR | S_IWUSR); > sh_mem = (char *)shmat(segment_id, NULL, 0); > > mprotect(sh_mem + 2*PAGE_SIZE, PAGE_SIZE, PROT_NONE); > > mprotect(sh_mem + PAGE_SIZE, PAGE_SIZE, PROT_WRITE); > > mprotect(sh_mem + 2*PAGE_SIZE, PAGE_SIZE, PROT_WRITE); > > shmdt(sh_mem); > > shmctl(segment_id, IPC_STAT, &shmid_ds); > printf("nattch after shmdt(): %lu (expected: 0)\n", shmid_ds.shm_nattch); > > if (shmctl(segment_id, IPC_RMID, 0)) > printf("IPCRM failed %d\n", errno); > return (shmid_ds.shm_nattch) ? 1 : 0; > } > > Fixes: 714965ca8252 ("mm/mmap: start distinguishing if vma can be removed in mergeability test") > Signed-off-by: Vlastimil Babka > Reported-by: Michal Hocko > Cc: Liam R. Howlett > Cc: Lorenzo Stoakes > Cc: > --- > v2: deduplicate code, per Lorenzo > mm/mmap.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index d89770eaab6b..3281287771c9 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -954,13 +954,21 @@ static struct vm_area_struct > } else if (merge_prev) { /* case 2 */ > if (curr) { > vma_start_write(curr); > - err = dup_anon_vma(prev, curr, &anon_dup); > if (end == curr->vm_end) { /* case 7 */ > + /* > + * can_vma_merge_after() assumed we would not be > + * removing prev vma, so it skipped the check > + * for vm_ops->close, but we are removing curr > + */ > + if (curr->vm_ops && curr->vm_ops->close) > + err = -EINVAL; > remove = curr; > } else { /* case 5 */ > adjust = curr; > adj_start = (end - curr->vm_start); > } > + if (!err) > + err = dup_anon_vma(prev, curr, &anon_dup); > } > } else { /* merge_next */ > vma_start_write(next); > -- > 2.43.1 > Looks good to me, feel free to add: Reviewed-by: Lorenzo Stoakes