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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,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 CD199C433C1 for ; Fri, 26 Mar 2021 12:08:29 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 27F3861A2A for ; Fri, 26 Mar 2021 12:08:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 27F3861A2A 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 DBBF76B0070; Fri, 26 Mar 2021 08:08:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D93236B0071; Fri, 26 Mar 2021 08:08:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C59D46B0072; Fri, 26 Mar 2021 08:08:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0071.hostedemail.com [216.40.44.71]) by kanga.kvack.org (Postfix) with ESMTP id A81106B0070 for ; Fri, 26 Mar 2021 08:08:27 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 62F97173085F for ; Fri, 26 Mar 2021 12:08:27 +0000 (UTC) X-FDA: 77961903054.12.D4917A5 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf05.hostedemail.com (Postfix) with ESMTP id 24AA1E005F01 for ; Fri, 26 Mar 2021 12:08:26 +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=6CNIiT7q+Ups+0Vc8DJQD9WSyDH1NQMZhuJ6B6CQvG0=; b=AErjpU/dIo4bIDLo86LL6AWQL2 HyE8BVIZTayV2FqKgSgtRiLrJF/00Hm954Qx1zGZnzw/zMfyyN4EMfyFqq9MfwZIZNya2ibjjkmQb 1dxMOwVOpBScbtr1f4cJaxnSHm3hvyW3qOTGsuXkoQ6zSGilHwK5VhPAfCiRRrdkk/LW3cvrMs+KJ KZOLms9/Wv+Wi22Un8X4Bs+JyeK3GiBEm/99pWtCepRYXol+0gisuuaJ5XzkaNDGgktER4R5wf9j1 XrH7alkNmm2/q8hNsInB6CHqj7TbsaaqibnOxe37+AbIWuDgWGwSkKiOSwJaDN9Qa/rwkO9q8NdH9 6yRI+fyw==; Received: from willy by casper.infradead.org with local (Exim 4.94 #2 (Red Hat Linux)) id 1lPlFg-00Eljt-Pn; Fri, 26 Mar 2021 12:07:48 +0000 Date: Fri, 26 Mar 2021 12:07:44 +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: <20210326120744.GD1719932@casper.infradead.org> References: <20210319071547.60973-1-hannes@cmpxchg.org> <20210326025143.GB1719932@casper.infradead.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: 24AA1E005F01 X-Stat-Signature: o5tnyemqzfosk87kt441gbrpima8b451 Received-SPF: none (infradead.org>: No applicable sender policy available) receiver=imf05; identity=mailfrom; envelope-from=""; helo=casper.infradead.org; client-ip=90.155.50.34 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616760506-172375 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 09:04:40PM -0700, Hugh Dickins wrote: > On Fri, 26 Mar 2021, Matthew Wilcox wrote: > > 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(). I forgot to say earlier; I did add a test (lib/test_free_pages.c). Doubling it up to check GFP_KERNEL | GFP_ACCOUNT and GFP_KERNEL | GFP_COMP | GFP_ACCOUNT should be reasonable. > > 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. > > If I still believed in e320d3012d25, yes, that would look right > (but I don't have much faith in my judgement after all this). > > I'd fallen in love with split_page_memcg() when you posted that > one, and was put off by your #ifdef, so got my priorities wrong > and went for the split_page_memcg(). Oh, the ifdef was just a strawman. I wouldn't want to see that upstream; something like: unsigned long memcg_data = __get_memcg_data(page); ... __set_memcg_data(tail, memcg_data); with the appropriate ifdefs hidden in memcontrol.h would be my preference. > > > 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: > > Not confused, just pontificating from a misleading subset of the data. > I knew there's an even-more-history-than-tglx git tree somewhere, but > what I usually look back to is 2.4 trees, plus a 2.2.26 tree - but of > course that's a late 2.2, from 2004, around the same time as 2.6.3. I suspect it got backported ... https://github.com/mpe/linux-fullhistory/wiki is what I'm using for my archaeology, and it doesn't have the stable branches (1.0, 1.2, 2.0, 2.2, 2.4), so I don't know for sure. Anyway, my point is that the truly ancient drivers *don't* depend on this behaviour because the function didn't even exist when they were written.