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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 67EB1C433DB for ; Fri, 26 Mar 2021 02:52:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id ACA4861A38 for ; Fri, 26 Mar 2021 02:52:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ACA4861A38 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 2C0546B0036; Thu, 25 Mar 2021 22:52:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 270076B006E; Thu, 25 Mar 2021 22:52:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 111F86B0070; Thu, 25 Mar 2021 22:52:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0252.hostedemail.com [216.40.44.252]) by kanga.kvack.org (Postfix) with ESMTP id E9A726B0036 for ; Thu, 25 Mar 2021 22:52:24 -0400 (EDT) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 9E49A180ACEEB for ; Fri, 26 Mar 2021 02:52:24 +0000 (UTC) X-FDA: 77960501808.24.38B2512 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf14.hostedemail.com (Postfix) with ESMTP id D86A8C0007C2 for ; Fri, 26 Mar 2021 02:52:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=j0yXJ1+WE4VkDxmjzQaqjTpot8rkL4606fG9ZF6W9ek=; b=DnbaKodC7InpXKBwa/J3VzZTo/ sZyQf6z6uHDgh8ud6JwPuQXpkB+p54c2q5a4OT9GamfHHicwYwR7796R3L0c+QV1HZGlbXFPdQ9T9 J2k40pE/5jeV+XEW424SXtGRiZ2OH6NTeMUtxEvrk1FPbtUPJvEE8UUguLSGhJUHWFCvRMX1OChDE JjABgnP3XzCYGb38X/twcx4IzFl8dPka+ojqjD+qLrGyrjpk0uPU+4hV1fXOL9dRbeDHHy94Rg533 8VhVll7kRXvuj4l1hCEgTo/8gnKEluJebWpFp7JndpDv9MOrZm8cBktoJBRfjVTsLXNqB92kcH5pO 7hscQSpQ==; Received: from willy by casper.infradead.org with local (Exim 4.94 #2 (Red Hat Linux)) id 1lPcZb-00EAw0-Ju; Fri, 26 Mar 2021 02:51:51 +0000 Date: Fri, 26 Mar 2021 02:51:43 +0000 From: Matthew Wilcox To: Hugh Dickins Cc: Johannes Weiner , Andrew Morton , Michal Hocko , Zhou Guanghui , Zi Yan , Shakeel Butt , Roman Gushchin , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH] mm: page_alloc: fix memcg accounting leak in speculative cache lookup Message-ID: <20210326025143.GB1719932@casper.infradead.org> References: <20210319071547.60973-1-hannes@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: D86A8C0007C2 X-Stat-Signature: q3y4jdjuu119msg1cyjp9h1hkghfbs14 Received-SPF: none (infradead.org>: No applicable sender policy available) receiver=imf14; identity=mailfrom; envelope-from=""; helo=casper.infradead.org; client-ip=90.155.50.34 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616727140-225750 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: On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote: > The first reason occurred to me this morning. I thought I had been > clever to spot the PageHead race which you fix here. But now I just feel > very stupid not to have spotted the very similar memcg_data race. The > speculative racer may call mem_cgroup_uncharge() from __put_single_page(), > and the new call to split_page_memcg() do nothing because page_memcg(head) > is already NULL. > > And is it even safe there, to sprinkle memcg_data through all of those > order-0 subpages, when free_the_page() is about to be applied to a > series of descending orders? I could easily be wrong, but I think > free_pages_prepare()'s check_free_page() will find that is not > page_expected_state(). So back to something more like my original patch then? +++ b/mm/page_alloc.c @@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order) { if (put_page_testzero(page)) free_the_page(page, order); - else if (!PageHead(page)) - while (order-- > 0) - free_the_page(page + (1 << order), order); + else if (!PageHead(page)) { + while (order-- > 0) { + struct page *tail = page + (1 << order); +#ifdef CONFIG_MEMCG + tail->memcg_data = page->memcg_data; +#endif + free_the_page(tail, order); + } + } } EXPORT_SYMBOL(__free_pages); We can cache page->memcg_data before calling put_page_testzero(), just like we cache the Head flag in Johannes' patch. > But, after all that, I'm now thinking that Matthew's original > e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages") > is safer reverted. The put_page_testzero() in __free_pages() was > not introduced for speculative pagecache: it was there in 2.4.0, > and atomic_dec_and_test() in 2.2, I don't have older trees to hand. I think you're confused in that last assertion. According to linux-fullhistory, the first introduction of __free_pages was 2.3.29pre3 (September 1999), where it did indeed use put_page_testzero: +extern inline void __free_pages(struct page *page, unsigned long order) +{ + if (!put_page_testzero(page)) + return; + __free_pages_ok(page, order); +} Before that, we had only free_pages() and __free_page(). > So, it has "always" been accepted that multiple references to a > high-order non-compound page can be given out and released: maybe > they were all released with __free_pages() of the right order, or > maybe only the last had to get that right; but as __free_pages() > stands today, all but the last caller frees all but the first > subpage. A very rare leak seems much safer. > > I don't have the answer (find somewhere in struct page to squirrel > away the order, even when it's a non-compound page?), and I think > each of us would much rather be thinking about other things at the > moment. But for now it looks to me like NAK to this patch, and > revert of e320d3012d25. We did discuss that possibility prior to the introduction of e320d3012d25. Here's one such: https://lore.kernel.org/linux-mm/20200922031215.GZ32101@casper.infradead.org/T/#m0b08c0c3430e09e20fa6648877dc42b04b18e6f3