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 8EBFAC433F5 for ; Wed, 19 Jan 2022 23:28:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6C3956B0071; Wed, 19 Jan 2022 18:28:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6732A6B0073; Wed, 19 Jan 2022 18:28:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 53AB86B0074; Wed, 19 Jan 2022 18:28:45 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id 45D626B0071 for ; Wed, 19 Jan 2022 18:28:45 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id F1D651812C1C4 for ; Wed, 19 Jan 2022 23:28:44 +0000 (UTC) X-FDA: 79048628610.13.381D95D Received: from mail-ua1-f50.google.com (mail-ua1-f50.google.com [209.85.222.50]) by imf26.hostedemail.com (Postfix) with ESMTP id A6D28140003 for ; Wed, 19 Jan 2022 23:28:44 +0000 (UTC) Received: by mail-ua1-f50.google.com with SMTP id r15so7637724uao.3 for ; Wed, 19 Jan 2022 15:28:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SAvFOz8ghr5oAZyEvyWgFSjE0cOqBv8pLZgD3az/Alw=; b=bsrpLBeEmGPON4IVnn1J9LS41vfyBHVbscaKWsuCglNOrJmCd5wbVfUTccva0jnF4t 279q+9zbrD+ZWr8qrelwJQTRhNzSYbs5gig4TBG0nyZEqIinSkCPAxiYOvna0hrEiv5D xoeJC9NwShyhujgekVKeZa14J68SMjQo3LSdBUda1rDhWMATtytzbjfPJCNxcthKatxB wt70R5w+8MsTAylz4/5aRRwYbrOpRn5RU8pogPtfenu6omPyh9m0vxuWpD+rg4Rk5VS2 Cm+SF06WzCepcaQQeTQ12imr97K+3TkPsU54aR6k7wXd2b9y4OVXuKL4xdbvd/7zw4B+ P/YQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SAvFOz8ghr5oAZyEvyWgFSjE0cOqBv8pLZgD3az/Alw=; b=rTU2UDBOWi+2muYr2HGvyBW9YquUJ2FApOQD6+cZ8gFyPaGJFSABdy4fQKcq6dgH9B wMUVfaefJ+3EMXu19iPrE9RnZbBPqm0A+PZewWtZ1/MU7QMNzXYigGiwxzOTd5MP1VUi Ph5FrU9dhlHZgCQ0xMshk5I1c6HcdYiJWBq6KQz8BEVB2ni94IZ4vzOj1mM+u7y+BlEp PhMkurltsUDT6/ax0Xhjzk34ulNc+0mZHIMw8MkwwT4tweBniqS4Z+NdcWhmX9o1BMA4 hpNvdxbsPPOGljhzDr0l5q9IWjHc4M3ZgrDSxc+C6OdRFV5Rl4oyWYOCiYEjRDQfr72s XZJA== X-Gm-Message-State: AOAM532teyjBCyQlY9h+f3VDAUxYrorZYzQL0Efj0pxthD2t1QfIUbV6 4/bimttf6gQjX14zFtLMKhp8yCaAAnRnCtYhm+SLfg== X-Google-Smtp-Source: ABdhPJztNtP2ASrwQij4Hnq8zUdL8VnM6LFy0K4AN9sQGIkxltMSEPOR/5D8UuhwRg2Dmn31bBqJ3tpesxaIKOurOlI= X-Received: by 2002:a67:6587:: with SMTP id z129mr140195vsb.61.1642634923681; Wed, 19 Jan 2022 15:28:43 -0800 (PST) MIME-Version: 1.0 References: <20220118230539.323058-1-pcc@google.com> In-Reply-To: From: Peter Collingbourne Date: Wed, 19 Jan 2022 15:28:32 -0800 Message-ID: Subject: Re: [PATCH] mm/mmzone.c: fix page_cpupid_xchg_last() to READ_ONCE() the page flags To: Peter Zijlstra Cc: Andrey Konovalov , Andrew Morton , Linux Memory Management List , Linux Kernel Mailing List , Mel Gorman , "# 3.4.x" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: A6D28140003 X-Stat-Signature: 9fozh9o3d9e565pgh1oqxiodtbpjfxn7 Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=bsrpLBeE; spf=pass (imf26.hostedemail.com: domain of pcc@google.com designates 209.85.222.50 as permitted sender) smtp.mailfrom=pcc@google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1642634924-553447 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 Wed, Jan 19, 2022 at 2:03 AM Peter Zijlstra wrote: > > On Tue, Jan 18, 2022 at 03:05:39PM -0800, Peter Collingbourne wrote: > > After submitting a patch with a compare-exchange loop similar to this > > one to set the KASAN tag in the page flags, Andrey Konovalov pointed > > out that we should be using READ_ONCE() to read the page flags. Fix > > it here. > > What does it actually fix? If it manages to split the read and read > garbage the cmpxchg will fail and we go another round, no harm done. What I wasn't sure about was whether the compiler would be allowed to break this code by hoisting the read of page->flags out of the loop (because nothing in the loop actually writes to page->flags aside from the compare-exchange, and if that succeeds we're *leaving* the loop). That could potentially result in a loop that never terminates if the first compare-exchange fails. This is largely a theoretical problem as far as I know; the assembly produced by clang and gcc on x86_64 and arm64 appears to be doing the expected thing for now, and we're using inline asm for compare-exchange instead of the compiler builtins on those architectures (and on all other architectures it seems? no matches for __atomic_compare_exchange outside of kcsan and the selftests) so the compiler wouldn't be able to look inside it anyway. > > Fixes: 75980e97dacc ("mm: fold page->_last_nid into page->flags where possible") > > As per the above argument, I don't think this rates a Fixes tag, there > is no actual fix. Okay, I'll remove it unless you find the above convincing. > > Signed-off-by: Peter Collingbourne > > Link: https://linux-review.googlesource.com/id/I2e1f5b5b080ac9c4e0eb7f98768dba6fd7821693 > > That's that doing here? I upload my changes to Gerrit and link to them here so that I (and others) can see the progression of the patch via the web UI. > > Cc: stable@vger.kernel.org > > That's massively over-selling things. Fair enough since it isn't causing an actual problem, I'll remove this tag. > > --- > > mm/mmzone.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/mmzone.c b/mm/mmzone.c > > index eb89d6e018e2..f84b84b0d3fc 100644 > > --- a/mm/mmzone.c > > +++ b/mm/mmzone.c > > @@ -90,7 +90,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid) > > int last_cpupid; > > > > do { > > - old_flags = flags = page->flags; > > + old_flags = flags = READ_ONCE(page->flags); > > last_cpupid = page_cpupid_last(page); > > > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > > I think that if you want to touch that code, something like the below > makes more sense... Yeah, that looks a bit nicer. I'll send a v2 and update the other patch as well. Peter