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 5448DC3DA6D for ; Fri, 23 May 2025 10:44:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A44846B0088; Fri, 23 May 2025 06:44:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A1C4D6B009A; Fri, 23 May 2025 06:44:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 958BE6B00CD; Fri, 23 May 2025 06:44:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 76EAD6B0088 for ; Fri, 23 May 2025 06:44:52 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 218888201C for ; Fri, 23 May 2025 10:44:52 +0000 (UTC) X-FDA: 83473839624.13.4541762 Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by imf23.hostedemail.com (Postfix) with ESMTP id 1A56B14000D for ; Fri, 23 May 2025 10:44:49 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=b2a5Tg+5; spf=pass (imf23.hostedemail.com: domain of rcn@igalia.com designates 213.97.179.56 as permitted sender) smtp.mailfrom=rcn@igalia.com; dmarc=pass (policy=none) header.from=igalia.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1747997090; 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=0aeFl53JyQJoFv9ov6KZzE95rLNvmraObXYehnksakE=; b=u+YOv5xzrkb4cnDhSWf8baA1SooVNPLeF8OBDWOGkEXVX0AazHsShWo96ZOjNpRoS0dnQZ 2JyvobwKpM1w5gdr+UBYtk/EVyN84dWPXrbkpBRpsSIe9+rvLeF0XqMMucl4zgF0ispi4e XklN95pDJ2RKnMAVqH92dyP+i9LQEa0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1747997090; a=rsa-sha256; cv=none; b=fhhTpkBRNf5AAF6TejLxYmVaWxIstHuytV+T2lvdQJA0PpVq62J4dvhgN/0SUQzgCcfSN4 Genw9MfyB6YzIsgjjo6JlbQLsJE7cz5Qdha4fYLcKwGqnI9QZT84fKOIy5Uioc5UrjpkkN IyprICWBAvM5Mh8jsmKGcGTifz8K2k8= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=b2a5Tg+5; spf=pass (imf23.hostedemail.com: domain of rcn@igalia.com designates 213.97.179.56 as permitted sender) smtp.mailfrom=rcn@igalia.com; dmarc=pass (policy=none) header.from=igalia.com DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Type:MIME-Version:Message-ID:Date:References: In-Reply-To:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=0aeFl53JyQJoFv9ov6KZzE95rLNvmraObXYehnksakE=; b=b2a5Tg+5HvcXY0Oo9bgdU5lqVJ 4zu4Ka/CeNw3mwz2dNnTOV7XJW7h3yLffXg0AINfWCiUs50mzbdnJAuEC83TNz+xaCDf4K/ofuA+H wItKnyG5zg2L81ua6o1ApGZ43wcvqd7HfDC4p9FUIox2bbZd8lsk1qlpiAu1nBPF14HdTzd0YmRXk hxnD0C+go2X3eGCgctOMP38oWzK2JEBXFXNa9PtNFbEXfdIhwMerlGgf4Ywmrp+HOenx6HiPvY/hj GtUlO7SvjoT2XzV01/y7SeKkfHu2g9H/El2BttesQUGH8sFKDZNmYV5zEHFm9vUYMoglANwzekTLb wjUFRWBg==; Received: from 53.red-81-38-30.dynamicip.rima-tde.net ([81.38.30.53] helo=localhost) by fanzine2.igalia.com with utf8esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim) id 1uIPtH-00C8SI-Si; Fri, 23 May 2025 12:44:39 +0200 From: Ricardo =?utf-8?Q?Ca=C3=B1uelo?= Navarro To: Lorenzo Stoakes Cc: Andrew Morton , "Liam R. Howlett" , Vlastimil Babka , Jann Horn , Pedro Falcato , revest@google.com, kernel-dev@igalia.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Oscar Salvador Subject: Re: [PATCH] mm: fix copy_vma() error handling for hugetlb mappings In-Reply-To: References: <20250523-warning_in_page_counter_cancel-v1-1-b221eb61a402@igalia.com> Date: Fri, 23 May 2025 12:44:32 +0200 Message-ID: <87iklrbo8f.fsf@igalia.com> MIME-Version: 1.0 Content-Type: text/plain X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 1A56B14000D X-Stat-Signature: uexmy5bnkk9hpkr8kotpaeh5bspnetar X-Rspam-User: X-HE-Tag: 1747997089-495085 X-HE-Meta: U2FsdGVkX1/KUCE25v2b4h1Zovh1OqQ2+PsOK/idB0/Fftjk2KqWnYqUkEnn19R8C4B0XCKEjRpLJ+n6ABVfcqJUsOiw9Ebq5wtYUFkQFcr/pjslMv3uOOvPm8FtjlaPREJ20k55kPeBncjVMexZQ4ioKPSOFOAM7FBBfD4bdAV9icek0JKwYsAhawpTicau4JgfLmeuXXE4xjBvaTqREC+806d5IcKKO79mw5etP+2dDUEoqljDveSjnIPowTB2wc+nY2IRHdUjFx6jNP7OJOsh3LIYl0p46Nn6y2JvDdL0LIdrcc3elrOdcYWD7RKEgwwA0bQwTq7sJdkWD5ST/TPFwU7GckSrk1j7dgsyaeIY9QeguAu5G5eHybV8ZfeEtxqIpfLQ6dt0hazdD99YJ5Vl0AdJ8kEgnVGulPe9R6Bi1u9OFWuGGa38aI3fY4ZfbLk83SvXWrgEnnkse7H9UQKP9r9e2IOHkfoiXeVe579ke+3S8Ineswz0LnfxjsYF838EGQTxOd3tofqc3s+zH9GuWh7iDRyWn1cr4gfBvELNCLj1h/ATv+fl8NhaN7doY3VFILxvToR8RgvAGhkaoQ1uFKdPnrRZpoyBozkWHBfBRiqqvi5md7DRdn7tu7T+XY38nKd7xcQwEVJKTKw9QsKESQ5iHhDjHoD9c8q7NBvNqQiarq0Z3gfexNRboaDylJAOWPV+zlMVDyO9KItOliFYXBboxhtLDSCNtI58zzXUO3Ynrl5IQgwkqqXoo30oeWegXmCS4309I3mJLoHrdAjfc8maVMEWjtgaB7F7bqAdhjqTW8QCCso8Zixwh59Uwrj1x28xlbgxNC3JUtGuvItu97+4INkPCI/5lcsZphyjptTwwy+6jXrwVpbxlhVRNb3l6rNhb4jB1IkjaCNoO5BWjxszNbRm+3wOq/o0jHfjCc0O3dX2GsDdDXrLQ4oCw2v9nlb68b4OgsBqWSI aHG7yCPw eVFoRtcduOCZdCxC1TvbUI+XWaaBE9V7RCSmKuc17Dn6f6pHGtXQIWaURPRTgT0ZUMuGSuk04VH6lus/m5A1IGjqA2St7tVOxgOH5yYMpiUO19Q2TYDDdey27H6R/Fze7Bn9PTv5b8WWDpwPMef1FNPAlhHheEixe6dJZFQd6FSArjllEdO7LdepUKSGFV8XDWJcqQkijVcqWfXcdXvECxjv35+dHEtqoYEg2Po7ftfe7w2G8o9YWL/FynlVSR5QAj1Xi75lBmqlY578+t6qbgh+Tk1Gt/8fNl7YMf6BwYuldEx5z/uMmSX0nvg== 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: Hi Lorenzo, Thanks for the in-depth review! answers below: On Fri, May 23 2025 at 11:00:40, Lorenzo Stoakes wrote: > OK so really it is _only_ when vma_link() fails? AFAICT yes, since copy_vma() only calls vma_close() if vma_link() fails. A failure in any of the other helpers in copy_vma() before it is handled by simply freeing the allocated resources. > Ordinarily 'private syzbot instance' makes me nervous, but you've made your case > here logically. I understand your qualms with that but, although that instance is mostly concerned with downstream code, in this case there's nothing unusual, as it was able to find the issue in mainline with a common reproducer. The closest public report I found was the one I linked in [3], although I couldn't reproduce the issue with the repro provided there. > Hm, do we have a Fixes? I couldn't find a single commit to point as a "Fixes". The actual commit that introduces that close_vma() call there is 4080ef1579b2 ("mm: unconditionally close VMAs on error") although I wouldn't say that's the culprit. As you said, the problem with vma_close() seems to be more involved. If you want me to add that one in the "Fixes" tag so we can keep track of the context, let me know, that's fine by me. > Why 6.12+? It seems this bug has been around for... a while. Because in stable versions lower than that (6.6) the code to patch is in mm/mmap.c instead, so I'd rather have this one merged first and then submit the appropriate backport for 6.6. > Thanks for links, though it's better to please provide this information here > even if in succinct form. This is because commit messages are a permanent > record, and these links (other than lore) are ephemeral. True but, as you said, it's a bit of a pain to try to fit all the info in the commit message, and the repro will still be living somewhere else. > So, can we please copy/paste the splat from [1] and drop this link, maybe just > keep link [2] as it's not so important (I'm guessing this takes a while to repro > so the failure injection hits the right point?) and of course keep [3]. Sure, I'll make the changes for v2. FWIW, in my tests the repro could trigger this in a matter of seconds. > So, > > Could you implement this slightly differently please? We're duplicating > this code now, so I think this should be in its own function with a copious > comment. > > Something like: > > static void fixup_hugetlb_reservations(struct vm_area_struct *vma) > { > if (is_vm_hugetlb_page(new_vma)) > clear_vma_resv_huge_pages(new_vma); > } > > And call this from here and also in copy_vma_and_data(). > > Could you also please update the comment in clear_vma_resv_huge_pages(): > > /* > * Reset and decrement one ref on hugepage private reservation. > * Called with mm->mmap_lock writer semaphore held. > * This function should be only used by move_vma() and operate on > * same sized vma. It should never come here with last ref on the > * reservation. > */ > > Drop the mention of the specific function (which is now wrong, but > mentioning _any_ function is asking for bit rot anyway) and replace with > something like 'This function should only be used by mremap and...' Ack, thanks for the suggestions! Cheers, Ricardo