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 45171C352A3 for ; Mon, 10 Feb 2020 13:55:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 0614A206ED for ; Mon, 10 Feb 2020 13:55:58 +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="StiymXhX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0614A206ED 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 81F346B00FA; Mon, 10 Feb 2020 08:55:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A9EE6B00FB; Mon, 10 Feb 2020 08:55:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6713F6B00FC; Mon, 10 Feb 2020 08:55:58 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0034.hostedemail.com [216.40.44.34]) by kanga.kvack.org (Postfix) with ESMTP id 4BA546B00FA for ; Mon, 10 Feb 2020 08:55:58 -0500 (EST) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id F40F63AB9 for ; Mon, 10 Feb 2020 13:55:57 +0000 (UTC) X-FDA: 76474365996.02.low12_8b5803465f136 X-HE-Tag: low12_8b5803465f136 X-Filterd-Recvd-Size: 7536 Received: from mail-qt1-f194.google.com (mail-qt1-f194.google.com [209.85.160.194]) by imf29.hostedemail.com (Postfix) with ESMTP for ; Mon, 10 Feb 2020 13:55:57 +0000 (UTC) Received: by mail-qt1-f194.google.com with SMTP id n17so5163793qtv.2 for ; Mon, 10 Feb 2020 05:55:57 -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=uvc+HCAjGfX6aDuiEDkX1stZqSNz9kTlsYc/8zRaw1M=; b=StiymXhXU8O8XtYolLxmQCpaPboJPGmag5L97rZ1HtVEAypI2ojif8OJMfrxHNxGgP f9TeY01de6tYRd/pz9atN9bfseymq13x1E+bI6DyQJ8gGLeDqjDnoSbHF5oUvs8IKmWf uIp8m0FiL6gKKn/bQ62WMwVcur18Vk8JTz4sSl91hVOjU7uKqukzQAp2iRVZ3t6rT0Sl I+b5cPeX2vr0DNr6ik7kYoZsPIbpl2RPSho8osX5lOzKvqSL3KensYUwzX/C24EYOYRh VphGzULJ6PyiOElvDG8i+swpGjQcYJ62V0iY96ZpW34GL6gmc65lxFhaScgdb7nrWt2/ wmsw== 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=uvc+HCAjGfX6aDuiEDkX1stZqSNz9kTlsYc/8zRaw1M=; b=BWK5RTT7QB3rF1spIiuiH7qQCyGt0Dybk2bnyYtCdTITmwSTvPKj+fbj4A5Mf8+v6F RmUGMIC8eA1vpC745C/I9uXbuN2H++gZuscnO28zOdmHbyVZzhDvAEqF/9Xolkow+1VT KUsSil+KjjqotkoPZdXKKM/SA0tSTazMTR5q0rNKlDD2A1v8fI9Wb1jD3lv43s/wDNFo RGlosUeQUlOF8VhMXeCTbrzm25qLHkID8Ivmo9SSv37cDUdNiNdJepXVAkxmywcwm3rJ yaxjwOmNSImymyAOne382yrrxicbUOHNLdlqNd39+RpxRDGQbgv+wNhNlsylaVBE4618 LFIQ== X-Gm-Message-State: APjAAAX4oGCZ8s82kR5j6ArWx7rW8mLtNfDLWKMebajMjlpkpenA5uLS OuC1p0puL+XC9Y8J+CHJrJVeD4aa7SeFmQ== X-Google-Smtp-Source: APXvYqxHJugKywFkG5dTaHKtVSfIc+JAdwSQurvj8aM2E0xJYwwDdipVzimIaYrkcQ+CX1qgGcwGVQ== X-Received: by 2002:aed:3eee:: with SMTP id o43mr10143047qtf.33.1581342956768; Mon, 10 Feb 2020 05:55:56 -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 a24sm157600qkl.82.2020.02.10.05.55.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Feb 2020 05:55:56 -0800 (PST) Message-ID: <1581342954.7365.27.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 08:55:54 -0500 In-Reply-To: References: <26B88005-28E6-4A09-B3A7-DC982DABE679@lca.pw> <1581341769.7365.25.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 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 wro= te: > > > > >=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 access th= ose > > > > > bits and safe w.r.t. data races. > > > > >=20 > > > > > For example, this may be used when certain bits of @flags m= ay > > > > > 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 *pa= ge) > > > > > { > > > > > + ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_= 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 verif= y 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 race = report > > > > > is generated. > > > > > 4. If somebody modifies ZONES bits concurrently, KCSAN will tel= l 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 does no= t feel me any better than data_race() with commenting occasionally. > > >=20 > > > Point 4 above: While data_race() will ignore cause KCSAN to not rep= ort > > > 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, but ju= st > > > remember that if you decide to silence it with data_race(), you nee= d > > > 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_BIT= S() in mind > > for other places. >=20 > Right now there are no concurrent writers to those bits. But somebody > might introduce a bug that will write them, even though they shouldn'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 reason. Surely, we could add many of those to catch theoretical issues. I can thi= nk of more like ASSERT_HARMLESS_COUNTERS() because the worry about one day some= one might change the code to use counters from printing out information to ma= king important MM heuristic decisions. Then, we might end up with those too ma= ny macros situation again. The list goes on, ASSERT_COMPARE_ZERO_NOLOOP(), ASSERT_SINGLE_BIT() etc. On the other hand, maybe to take a more pragmatic approach that if there = are strong evidences that developers could easily make mistakes in a certain = place, then we could add a new macro, so the next time Joe developer wants to a = new macro, he/she has to provide the same strong justifications? >=20 > > >=20 > > > There is no way to automatically infer all over the kernel which bi= ts > > > 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