From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f53.google.com (mail-ot1-f53.google.com [209.85.210.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9A92347DD6B for ; Tue, 14 Jul 2026 15:39:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784043552; cv=none; b=G9pUm+5+nYZkXPA+cRDBi0GNy1YAdBkgvW2RcE3BbxA6zPyUBDW6avbkZ1vOdtrJoWFXunSYxZJvjopr5ErBLvz2Bv/ZrjOItT4MunzoZWEg410wRbixavN1U7/Y4RjAClZIS1/IjnrloalEH0FyOyzvgbYQG1527GEnPLmKH40= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784043552; c=relaxed/simple; bh=2p32/JOiOUGkzMruOJtrpMcYlWH5TB77HdwhMlQYP5g=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=C21I22Ay3PTE6AnJtm0rrUlrKptrrNpAuW+vbKF2vw2aLdRzvRlrSUiIRpP1zHStOTxbQ9Tvy9yDOvCAbD5vIs9OVB88yXFj6lFL5QzJUOc++GPKq/59TZKn8Ooa4XAVfXCNZI70ZxcieiBdGFLkDk7Vs5qiDKte0nFKfhJ0zRc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=GcCVT0kz; arc=none smtp.client-ip=209.85.210.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="GcCVT0kz" Received: by mail-ot1-f53.google.com with SMTP id 46e09a7af769-7e9f829d75aso726700a34.0 for ; Tue, 14 Jul 2026 08:39:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1784043549; x=1784648349; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to:content-type; bh=h2jZPg8BmRJ0mbFQa7ElFHkaNBrmZCQEMsa0xC0hHN8=; b=GcCVT0kzYiQOrqrtVMyNzTe9FRn8eXNMmPigxErccptVbBohoP8WZt01cd+GmEbGfR nN2RvqBjYhkRRnN9tf/mY7dGgh6l4GwGiQc5d4l32id3DgdJKPc9h9YZQLGajjGrK4GT ONZTRGpUPU63ow4/VmXijus1pXAZYjNQF67s1QllfPfzIH+XGf/Oir5OCuHvBxFLSYE8 eIaDggUt6bz0Ujt4XDi+AdOT+koNePhMaMnNGUuLwWol41LltE1haqQx2OJaPeOaqjCf g6hrgZNo7rSta4LpL0ePACQnQA4lJFD80DA5WJkxWkeEitcJHXOHRH6rVMZwpK2tiGlD 9+9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784043549; x=1784648349; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to:content-type; bh=h2jZPg8BmRJ0mbFQa7ElFHkaNBrmZCQEMsa0xC0hHN8=; b=Ku32isru1d6z4TOMWgTEcFJg8HZNxm6A/UEzT6UXio7bbAoizTcLU1dbqUMuBFkMpG iGaznKFzFKn/KgttIrSZXDfNhy7u5M6w1vG8gWQu1GkTnH1hwEjZZIWc7B5AsiuIswqA u94ReKonA66QUYLMqZobKHF005oa8WejbxV+jqSqz6qF9Mvh8lcOd85XI9AnBznkAdm4 iv0Q6n2UIl2cDfuOKsnD/3RYVlV3Lmu/QjBlLBYMBb9Lduq/JMs3HO0GS8wGfFHGEDZs wEX1/Slxq408UnKaGj+K4jk2HVT9MgXcG30XCN/LDwkqK+D7uJWbsfKfAlOd53FLZvCJ Hvuw== X-Forwarded-Encrypted: i=1; AFNElJ8GHKgMRBHEWfZbeQyMwlnirz7oEL1rIunDzZ3MOI/K4bNPsvDyPf8wrxEiE3S0O+jxclpw716+aXEFeJI=@vger.kernel.org X-Gm-Message-State: AOJu0Yx5Hkq9inDngPKoNCns6RH8tiq5Q7/6wcVf+WhFWq/gDg4L9u7U Q5RuUbiGvdIyhYRQ0iUm7IVYUB0paRVB07rzrUHrruG5rdYri9BdyYT6 X-Gm-Gg: AfdE7clbgYPc/xy2HBbI0n3/KAC7/d7lMVVK1/pPRUwRWBXHBpWDgZlH/8J5rlrLLPC FCHSNvcI/B9txyya62lKqHD0dDEOsvcCiwUBFIQhpiydXlW53szR7D7jTuoLzYwOvKARJ8mODYl i35yxP71jVwOQSf2ff3Fq86O5AgTOrFAngGMmxszdLI1Y4cHO+pQdx2u6aId641Lqn5hJFQ1VgS rzbWSX98m66yO0XdPcZaiIqWy0ytqZISa+jOlGDQlcFpY+6wcvVaYefpofNu2uB/JSG+YQTMkSK VlC6DvOSS5FAo/DQ6d/plP66Dtj6FJoLSuCROgrw7ghbspDrZipJO9ZzQu+zQIhgjkKSPbd3uFR AStDXSXZyMo1uwPXMHPIy7vFAiOYAXPmA72oGiNiDVh8ruu9piUijtBU7GvVHcpufuPgZGutbD4 T8Dhy2YO9YPREsQ2I/t8zOeFFF78my7q97R4S0r3bulEY= X-Received: by 2002:a05:6830:6ae6:b0:7e6:e385:4c1e with SMTP id 46e09a7af769-7ec096027c1mr8121305a34.4.1784043549120; Tue, 14 Jul 2026 08:39:09 -0700 (PDT) Received: from localhost ([2a03:2880:10ff:5f::]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7ebcb3f2b86sm15347657a34.26.2026.07.14.08.39.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jul 2026 08:39:08 -0700 (PDT) From: Joshua Hahn To: Ackerley Tng Cc: Ackerley Tng via B4 Relay , Muchun Song , Oscar Salvador , David Hildenbrand , Shakeel Butt , Nhat Pham , Andrew Morton , Peter Xu , Wupeng Ma , fvdl@google.com, rientjes@google.com, jthoughton@google.com, vannapurve@google.com, erdemaktas@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, David Carlier , Zhao Li , lance.yang@linux.dev Subject: Re: [PATCH v2 2/5] mm: hugetlb: Fix subpool usage leak on allocation failure Date: Tue, 14 Jul 2026 08:39:06 -0700 Message-ID: <20260714153906.772019-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.53.0 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit On Thu, 9 Jul 2026 15:15:50 -0700 Ackerley Tng wrote: > Ackerley Tng writes: > > > Joshua Hahn writes: > > > >> Hi Ackerley, > >> > >> Thank you for this series. I really wanted to work on hugeTLB accounting > >> fixes but never got the time to get to it. I'm very grateful that you > >> are taking a look!! > >> > >>> From: Ackerley Tng > >>> > >>> When alloc_hugetlb_folio() fails early (e.g. buddy allocation failure or > >>> hugetlb cgroup charging failure) and gbl_chg == 1 (meaning a reservation > >>> was not used, but a global page was allocated instead), the subpool page > >>> acquired via hugepage_subpool_get_pages() must still be returned. > >>> > >>> Currently, the error path out_subpool_put: only calls > >>> hugepage_subpool_put_pages() if !gbl_chg is true. If gbl_chg is 1, it > >>> skips it, permanently leaking the subpool's used_hpages counter. > >>> > >>> With the earlier patch to always track used_hpages in the subpool, always > >>> call hugepage_subpool_put_pages() if map_chg is true to consistently > >>> restore the page to the subpool. Only call hugetlb_acct_memory() to adjust > >>> global reservations if gbl_chg == 0 since gbl_chg == 0 indicates a > >>> subpool (and global) reservation was used. > >> > >> So I think that I've seen that this part of the accounting specifically > >> is a bit suspicious. There have been two attempts in the past to fix > >> this area [1] [2]. I think functionally they are quite similar to this > >> fix, they just open-code the contents of the put_pages function inside > >> the condition. I've Cc-ed the authors of those two patches in case > >> they wanted to chime in. > >> > > > > Thanks for connecting us! I didn't realize this was already being worked > > on. Also adding Lance, who commented at [3]. > > > >> I reference these fixes because I think they handle the minimum > >> subpage case a bit differently. To be honest, I recall reading those > >> fixes a while back and getting a bit confused on what exactly happens > >> when the page is absorbed to fulfill the minimum size... > >> > >> It does seem like Sashiko also notes this as a possible concern. > >> WDYT? Does your reproducer for this issue also work when a minimum > >> size is set (let's say, to 1?) > >> > > > > Let me look into this more! > > Hi Ackerley, sorry that the reply took a while : -( > I was looking at Sashiko's comment [4] and Sashiko is right that there > could be a race. Specifically, at the time of > hugepage_subpool_get_page(), I might be using a reservation, hence > returning 1 (thread A), but at the time of hugepage_subpool_put_page(), > some other thread (B) might already have restored a reservation to the > subpool. > > With used_hpages tracking within the subpool, we can rely on the return value of > hugepage_subpool_put_page(): > > + Return 1: Reservation should be returned elsewhere (pool has enough pages to > satisfy minimum, or doesn't track minimums) > + Return 0: Reservation does not need to be returned elsewhere > > But calling hugepage_subpool_put_pages(1) doesn't say anything about whether the > 1 (in this case, 1 doesn't always represent a page, since specifically on the > error cases, the page wasn't allocated) consumed a reservation or not. > > + gbl_chg == 0 means a reservation was used, and > + gbl_chg == 1 means a reservation *should be used*, but > + gbl_chg == 1 does not necessarily mean a reservation should be used, since > h->resv_huge_pages may not have been decremented yet. (See goto > out_subpool_put and goto out_uncharge_cgroup_reservation in > alloc_hugetlb_folio()) Yeah... this kind of tripped me up. > So, we need to track if h->resv_huge_pages-- happened (with some local variable > like reservation_consumed) > > | reservation_consumed | hugepage_subpool_put_pages() return value | > h->resv_huge_pages++ | > |----------------------|-------------------------------------------|----------------------| > | 1 | 1 | > Yes | > | 1 | 0 | > No | > | 0 | 1 | > No | > | 0 | 0 | > No | > > TLDR: replace !gbl_chg with reservation_consumed? This seems like quite a reasonable approach. It's new information that we can't derive from the existing flags we have. > <<== reservation_consumed = true > > if (!gbl_chg) { > folio_set_hugetlb_restore_reserve(folio); > h->resv_huge_pages--; > <<== reservation_consumed = true > } > > if (map_chg) { > long gbl_reserve = hugepage_subpool_put_pages(spool, 1); > > if (!gbl_chg) <<== this should be reservation_consumed > hugetlb_acct_memory(h, -gbl_reserve); > } > > I believe this depends on always tracking used_hpages in subpools, in order to > know for sure, just based on a number (with no page information), if the page is > supposed to use a reservation. > > > Perhaps related: does it make sense to flipping the reservation tracking > to instead track an "available" page count? This is basically the > mathematical opposite of rsvd_huge_pages, as in, > > available_pages = free_huge_pages - resv_huge_pages > > This way, there's no need to synchronize the number of reserved pages in > both the subpool and global hstate. > > It's quite confusing now, that reserved_pages-- can be either "used a > reservation" or "unreserve". If we track the opposite, available_pages-- > will definitely mean the page's availability is removed from the global > hstate and transferred to the subpool, and allocating page doesn't > involve updating both the subpool and hstate. I think I like the semantic change, since it does make the code a lot more obvious on why and where we are making the accounting changes. One fear that I have is that this churn could introduce some new subtle accounting errors, since we have been fixing this area for quite some time now. Let's audit very carefully to make sure that there are no new accounting errors that will be introduced : -) > One downside I can think of is that creating hugepages (surplus, or > promote/demote, or hotplug, or runtime echo 1 > nr_hugepages) would need > to update the global hstate's available_pages count, but I imagine that > would be less frequent than allocating huge pages? Yup I think so. You might be more familiar with the new usecase you are adding as part of the guest_memfd, but I have not really seen a usecase for hugeTLB that would require userspace to do many hugeTLB allocations (that THP wouldn't be able to serve). Having this accuracy seems to be much more important than the rare contention we will see. Thanks again for the fixes and ideas Ackerley! Looking forward to your new approach : -) Joshua > >> > >> [...snip...] > >> > >> > >> [1] https://lore.kernel.org/linux-mm/20260428113037.88766-2-enderaoelyther@gmail.com/ > >> [2] https://lore.kernel.org/linux-mm/20260515202902.461539-1-devnexen@gmail.com/ > > [3] https://lore.kernel.org/linux-mm/20260428113059.79001-1-lance.yang@linux.dev/ > [4] https://sashiko.dev/#/patchset/20260708-hugetlb-alloc-failure-fixes-v2-0-c7f27cbb462b%40google.com?part=2,