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=-2.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 autolearn=no 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 D2F2BC3F68F for ; Mon, 10 Feb 2020 14:31:17 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6EED32080C for ; Mon, 10 Feb 2020 14:31:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=lca.pw header.i=@lca.pw header.b="XXf5//gt" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6EED32080C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lca.pw Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B876F6B0100; Mon, 10 Feb 2020 09:31:16 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B38BC6B0101; Mon, 10 Feb 2020 09:31:16 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A26E86B0102; Mon, 10 Feb 2020 09:31:16 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0045.hostedemail.com [216.40.44.45]) by kanga.kvack.org (Postfix) with ESMTP id 866C86B0100 for ; Mon, 10 Feb 2020 09:31:16 -0500 (EST) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 3C68918C9 for ; Mon, 10 Feb 2020 14:31:16 +0000 (UTC) X-FDA: 76474454952.28.curve83_b069e1c9c826 X-HE-Tag: curve83_b069e1c9c826 X-Filterd-Recvd-Size: 8886 Received: from mail-qk1-f193.google.com (mail-qk1-f193.google.com [209.85.222.193]) by imf17.hostedemail.com (Postfix) with ESMTP for ; Mon, 10 Feb 2020 14:31:15 +0000 (UTC) Received: by mail-qk1-f193.google.com with SMTP id a2so6159012qko.12 for ; Mon, 10 Feb 2020 06:31:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lca.pw; s=google; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=qcAgZw/ui3CLohL6PVMtRlohm+XOB+CMbaeiCefL8Tw=; b=XXf5//gtn9ah4o1Dg3ynTtJvV9QlT9wOauHez+XKzTaZBmpDuv7nb/+Aq5XchwD5qp Cb5yGuRptx8TFxSSM0Ke++M0aRlmnhjBMX+wqCZ/mZuLeA8oRDwx2PAFYW2zQJnbGTYh orbTORk0adPsno9WyR3BbkHdALwJc7sln8jqXZjh6wO+pd1G5KXUO5rqHVcwUbU7B7eu BxYvkPhwwXewl/+rCl6Z34+jl1nTUAIaPHS3WZzLKXy4KmKB0J7Bg/fuTVl2WA/ovjRi VIIcKSKU4+e6S+E5MD1jRpQERWH34lyO4oJMdxrBREzLi4r5aVCzQPTa5MgFRiQNn8Gm NCjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=qcAgZw/ui3CLohL6PVMtRlohm+XOB+CMbaeiCefL8Tw=; b=XVPpy6DLpxP8iXdPww7GRcguUzx8p7gmIo4ypbMqKkbHgHc8IDfk80nkXzRxzVamxU a3u1qAZub7/l5m1mcGulIiXxPgdguS3Tj03Hm4uVP5SpVd9sxbZ40XXAOu8Oz/9sgGgB 20kiu8vaRSH1rz7ymTFeCVRBSdRq/aYdWUjjLAChR37yDqGJrs9Lvai3zUv42+XZmAHX 0IpCvotzuGmt9Y8+6/tTTUQaETE9EfHCwT3lGRs5Nnad3DhH2M/hXINmOEW4NuBpKA7L r93NFRy3tjlXHnoVe8PJ9JKOvr+AG5BZVH/n89Aa94Zgvs8ZOw0GuFqW6iZXBXUpBPIe g0HA== X-Gm-Message-State: APjAAAU3+bllGZtzSNdXfuK/2WOnbf9S251UU1NsgP1qNw+3gg622UJq AzuFXojzToK1fUFtcEAnJ8/H3A== X-Google-Smtp-Source: APXvYqzQj7JL3j6ulpB0erhHdZzp7Y5zaTnPaPrnXdAv/X9nGuhFlyVud31K6RxqiPcBhdidYvqYhQ== X-Received: by 2002:a37:270b:: with SMTP id n11mr1631942qkn.26.1581345074983; Mon, 10 Feb 2020 06:31:14 -0800 (PST) Received: from dhcp-41-57.bos.redhat.com (nat-pool-bos-t.redhat.com. [66.187.233.206]) by smtp.gmail.com with ESMTPSA id r12sm200397qkm.94.2020.02.10.06.31.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Feb 2020 06:31:14 -0800 (PST) Message-ID: <1581345072.7365.30.camel@lca.pw> Subject: Re: [PATCH] mm: fix a data race in put_page() From: Qian Cai To: Marco Elver Cc: John Hubbard , Jan Kara , David Hildenbrand , Andrew Morton , ira.weiny@intel.com, Dan Williams , Linux Memory Management List , Linux Kernel Mailing List , "Paul E. McKenney" , kasan-dev Date: Mon, 10 Feb 2020 09:31:12 -0500 In-Reply-To: References: <26B88005-28E6-4A09-B3A7-DC982DABE679@lca.pw> <1581341769.7365.25.camel@lca.pw> <1581342954.7365.27.camel@lca.pw> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-10.el7) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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 Mon, 2020-02-10 at 15:12 +0100, Marco Elver wrote: > On Mon, 10 Feb 2020 at 14:55, Qian Cai wrote: > >=20 > > On Mon, 2020-02-10 at 14:38 +0100, Marco Elver wrote: > > > On Mon, 10 Feb 2020 at 14:36, Qian Cai wrote: > > > >=20 > > > > On Mon, 2020-02-10 at 13:58 +0100, Marco Elver wrote: > > > > > On Mon, 10 Feb 2020 at 13:16, Qian Cai wrote: > > > > > >=20 > > > > > >=20 > > > > > >=20 > > > > > > > On Feb 10, 2020, at 2:48 AM, Marco Elver = wrote: > > > > > > >=20 > > > > > > > Here is an alternative: > > > > > > >=20 > > > > > > > Let's say KCSAN gives you this: > > > > > > > /* ... Assert that the bits set in mask are not written > > > > > > > concurrently; they may still be read concurrently. > > > > > > > The access that immediately follows is assumed to acces= s those > > > > > > > bits and safe w.r.t. data races. > > > > > > >=20 > > > > > > > For example, this may be used when certain bits of @fla= gs may > > > > > > > only be modified when holding the appropriate lock, > > > > > > > but other bits may still be modified locklessly. > > > > > > > ... > > > > > > > */ > > > > > > > #define ASSERT_EXCLUSIVE_BITS(flags, mask) .... > > > > > > >=20 > > > > > > > Then we can write page_zonenum as follows: > > > > > > >=20 > > > > > > > static inline enum zone_type page_zonenum(const struct page= *page) > > > > > > > { > > > > > > > + ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZO= NES_PGSHIFT); > > > > > > > return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK; > > > > > > > } > > > > > > >=20 > > > > > > > This will accomplish the following: > > > > > > > 1. The current code is not touched, and we do not have to v= erify that > > > > > > > the change is correct without KCSAN. > > > > > > > 2. We're not introducing a bunch of special macros to read = bits in various ways. > > > > > > > 3. KCSAN will assume that the access is safe, and no data r= ace report > > > > > > > is generated. > > > > > > > 4. If somebody modifies ZONES bits concurrently, KCSAN will= tell you > > > > > > > about the race. > > > > > > > 5. We're documenting the code. > > > > > > >=20 > > > > > > > Anything I missed? > > > > > >=20 > > > > > > I don=E2=80=99t know. Having to write the same line twice doe= s not feel me any better than data_race() with commenting occasionally. > > > > >=20 > > > > > Point 4 above: While data_race() will ignore cause KCSAN to not= report > > > > > the data race, now you might be missing a real bug: if somebody > > > > > concurrently modifies the bits accessed, you want to know about= it! > > > > > Either way, it's up to you to add the ASSERT_EXCLUSIVE_BITS, bu= t just > > > > > remember that if you decide to silence it with data_race(), you= need > > > > > to be sure there are no concurrent writers to those bits. > > > >=20 > > > > Right, in this case, there is no concurrent writers to those bits= , so I'll add a > > > > comment should be sufficient. However, I'll keep ASSERT_EXCLUSIVE= _BITS() in mind > > > > for other places. > > >=20 > > > Right now there are no concurrent writers to those bits. But somebo= dy > > > might introduce a bug that will write them, even though they should= n't > > > have. With ASSERT_EXCLUSIVE_BITS() you can catch that. Once I have = the > > > patches for this out, I would consider adding it here for this reas= on. > >=20 > > Surely, we could add many of those to catch theoretical issues. I can= think of > > more like ASSERT_HARMLESS_COUNTERS() because the worry about one day = someone > > might change the code to use counters from printing out information t= o making > > important MM heuristic decisions. Then, we might end up with those to= o many > > macros situation again. The list goes on, ASSERT_COMPARE_ZERO_NOLOOP(= ), > > ASSERT_SINGLE_BIT() etc. >=20 > I'm sorry, but the above don't assert any quantifiable properties in th= e code. >=20 > What we want is to be able to catch bugs that violate the *current* > properties of the code *today*. A very real property of the code > *today* is that nobody should modify zonenum without taking a lock. If > you mark the access here, there is no tool that can help you. I'm > trying to change that. >=20 > The fact that we have bits that can be modified locklessly and some > that can't is an inconvenience, but can be solved. >=20 > Makes sense? OK, go ahead adding it if you really feel like. I'd hope this is not the Pandora's box where people will eventually find more way to assert quanti= fiable properties in the code only to address theoretical issues... >=20 > Thanks, > -- Marco >=20 > > On the other hand, maybe to take a more pragmatic approach that if th= ere are > > strong evidences that developers could easily make mistakes in a cert= ain place, > > then we could add a new macro, so the next time Joe developer wants t= o a new > > macro, he/she has to provide the same strong justifications? > >=20 > > >=20 > > > > >=20 > > > > > There is no way to automatically infer all over the kernel whic= h bits > > > > > we care about, and the most reliable is to be explicit about it= . I > > > > > don't see a problem with it per se. > > > > >=20 > > > > > Thanks, > > > > > -- Marco