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=-19.8 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL 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 06ABEC433DB for ; Fri, 26 Mar 2021 04:05:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6C4EE619FC for ; Fri, 26 Mar 2021 04:05:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6C4EE619FC Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D91CC6B0036; Fri, 26 Mar 2021 00:05:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D41376B006E; Fri, 26 Mar 2021 00:05:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BE21E6B0070; Fri, 26 Mar 2021 00:05:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0002.hostedemail.com [216.40.44.2]) by kanga.kvack.org (Postfix) with ESMTP id A5C276B0036 for ; Fri, 26 Mar 2021 00:05:05 -0400 (EDT) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 6383C180931E9 for ; Fri, 26 Mar 2021 04:05:05 +0000 (UTC) X-FDA: 77960684970.08.9DBEA5E Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com [209.85.210.54]) by imf18.hostedemail.com (Postfix) with ESMTP id B9D8820001D6 for ; Fri, 26 Mar 2021 04:05:04 +0000 (UTC) Received: by mail-ot1-f54.google.com with SMTP id w21-20020a9d63950000b02901ce7b8c45b4so4087722otk.5 for ; Thu, 25 Mar 2021 21:05:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=xeJRI/bSrn1eC4w9Rqatogj3zx30m75cryPTZk0CO7o=; b=wIoSYann9vLV5TBBngoEF/fhWRXoVEs2/a/Nma/eiT81J6J79Ec1J2FRlo036zBd17 6j2v7Nrk+eQlCwdKc/b5X3Cn4GStVjxNug8VVdVnB66+vhUE4q1EvVG8NfXfCQIa4jnm 8Lkn0zKQX9xndeM9wR6TU2ecnWyT5QdGfDzcELVCrwlA0f/n9QTzWowTd3AWTFvEUuRX KSp5F435PbNcqtkv3K0aliXXoGKExDfDBAQ7LW24N8EPrdK5b3xeN370kioD4ps9ECS8 ibR60tg+18LUMuZ5XOLjeQvWY9Hd5WGylFGu6Sj7m6HPwE8lRARm/JLe0Ih09NibUU38 PEWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=xeJRI/bSrn1eC4w9Rqatogj3zx30m75cryPTZk0CO7o=; b=gCsJokpdBml61dNcDcUaR7C+QK4mUTnQfPAffadVb00em0RlTTfjvKz7MomOBy+0Pf pI/Fz6MkMz/xRD6S/d+Wr7Gq5FVi8A9XJFJI9t1eQH5YmCQ6fzh5RddEbqugSXUzjPKI s6d9CVbsp33/CYxeazYUHT9/ieM2YMRpiY7V506pexAi89lyIv8Q3CLyqI13xvyJmLpn S1VEnTewkf0pcAzoa9cl6kepzT2OY9QtfnKdCEBrCKUeVwLvL4/r1wfcDhW7MT1+Ob+b H7DhtS/Qp+SaD1vQMnVNF2B8LBX3W9y4iF/b2Txo7NMUapXnszhD+GVx+/qYMquZsMKD Hjgw== X-Gm-Message-State: AOAM5301jsUu45aBg/BwNPCM/0Q5UJonTCdb1vy+jWGGYjyiCJ+J3fyz FTG3LuE6/PqOj9u9lRZOpe698Q== X-Google-Smtp-Source: ABdhPJz44gh3DvCn74zpPfoS1tLJ52JmVUH8jZdUZkgM1yYxXyHp7Rs6+k+P716C96ZuSMbVe6Ptsg== X-Received: by 2002:a9d:d89:: with SMTP id 9mr10342469ots.23.1616731503946; Thu, 25 Mar 2021 21:05:03 -0700 (PDT) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id o197sm1719716ooo.44.2021.03.25.21.05.02 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Thu, 25 Mar 2021 21:05:03 -0700 (PDT) Date: Thu, 25 Mar 2021 21:04:40 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Matthew Wilcox cc: Hugh Dickins , 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 In-Reply-To: <20210326025143.GB1719932@casper.infradead.org> Message-ID: References: <20210319071547.60973-1-hannes@cmpxchg.org> <20210326025143.GB1719932@casper.infradead.org> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Stat-Signature: creb4famo6b4pt4z9ryaytixwko4sb5f X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: B9D8820001D6 Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf18; identity=mailfrom; envelope-from=""; helo=mail-ot1-f54.google.com; client-ip=209.85.210.54 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616731504-91131 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 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(). > > 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(). > > > 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. That tree shows a __free_pages() using atomic_dec_and_test(). But we digress... > > +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 Thanks for the link. And I'll willingly grant that your experience is vast compared to mine. But "Drivers don't do that, in my experience" is not a convincing reason to invalidate a way of working that the code has gone out of its way to allow for, for over twenty years. But you make a good point on the "Bad page" reports that would now be generated: maybe that will change my mind later on. Hugh