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 2BCD6C352A3 for ; Mon, 10 Feb 2020 16:23:15 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CA65720838 for ; Mon, 10 Feb 2020 16:23:14 +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="ot9S+A+S" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CA65720838 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 5DA976B0126; Mon, 10 Feb 2020 11:23:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5648E6B012B; Mon, 10 Feb 2020 11:23:14 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 42CBD6B012C; Mon, 10 Feb 2020 11:23:14 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 24A406B0126 for ; Mon, 10 Feb 2020 11:23:14 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id BB023181AC9BF for ; Mon, 10 Feb 2020 16:23:13 +0000 (UTC) X-FDA: 76474737066.15.use05_7369a86bb6e43 X-HE-Tag: use05_7369a86bb6e43 X-Filterd-Recvd-Size: 10089 Received: from mail-qk1-f194.google.com (mail-qk1-f194.google.com [209.85.222.194]) by imf38.hostedemail.com (Postfix) with ESMTP for ; Mon, 10 Feb 2020 16:23:12 +0000 (UTC) Received: by mail-qk1-f194.google.com with SMTP id p7so2096692qkh.10 for ; Mon, 10 Feb 2020 08:23:12 -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=TaHv+PKpHG2kLF+syKmgL65ROquUkDcK5zPG7XnFfj8=; b=ot9S+A+SKIJ10cvWpMVaaYrqqDKwI8Dvoav5W6xsvsS3mSLN93bsuMFLYPgaaoM16L REh4NMYoLn54LLM2uPn2Y5UjjveL9hDvu+PqWdWRBKKI7kZElzpCTvLF03fqqnOXRScw syq0lZDNglnKLhx0xeJ+MnVrD57lgsDsSatFYECRzNCemyQeuyh0pBQu9stbc8b3dvMB 8yWcVO/0+hW053plsoVi2kZbNbS0XptflPnhDN0jY176OHCtg1GPvW2LynN6wRyDItpV eF58NrHu5dK0BefbTWb/EZLivA+smyHY2sGhIAvWTxOgOVt9yrlGRmCBVlLckLWTwoED M+Bw== 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=TaHv+PKpHG2kLF+syKmgL65ROquUkDcK5zPG7XnFfj8=; b=VizWS3kfH24nXAGFx0tYMfrHs4FnxKXRaahyCmd1r/PIafP8C6H91/xcP2TxTwgNtv TTz41lJDkUgZayyMWt3VUel59qFayhd6g+pzwdzlljpL0RVtaq61ML01MkfWHWRyThN4 u1M2pz7hH3nmJ064sVtEeeVMRA+CAy3wj/kgSb7KsoGGMyff9myw8ZGScPZSCM2dM9ei yjpSw1Rp4KMb2pj5alX3iPk92ngP/Zym0slve4248eTVHnmtBmQQ4iGk4WPrgU9Iong0 dw9g9cCk6+e/dIWFEVUqp2DsFkMR1yNd8WrLV4t5XuT6naKTYTKtHVhPL7ZtAo6OpHp8 Yekw== X-Gm-Message-State: APjAAAV7jm8QIFLmldABOtdAYRzZBx+XICutvvu1bkg8Y6zJYa3bbpjp r7lwuU955VMUI6GQuqqgsWaUlA== X-Google-Smtp-Source: APXvYqxbU8J3b8OSVhklgbcprlJkDiq20U5N6jKEWVq0xbTU3s4v/Jkuk8Q08KyXs5BHcoN5lmJTsg== X-Received: by 2002:ae9:e306:: with SMTP id v6mr2094560qkf.162.1581351792284; Mon, 10 Feb 2020 08:23:12 -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 g6sm359335qki.100.2020.02.10.08.23.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Feb 2020 08:23:11 -0800 (PST) Message-ID: <1581351789.7365.32.camel@lca.pw> Subject: Re: [PATCH] mm: fix a data race in put_page() From: Qian Cai To: Marco Elver , John Hubbard Cc: 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 11:23:09 -0500 In-Reply-To: References: <5402183a-2372-b442-84d3-c28fb59fa7af@nvidia.com> <8602A57D-B420-489C-89CC-23D096014C47@lca.pw> <1a179bea-fd71-7b53-34c5-895986c24931@nvidia.com> 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 08:48 +0100, Marco Elver wrote: > On Sun, 9 Feb 2020 at 08:15, John Hubbard wrote: > >=20 > > On 2/8/20 7:10 PM, Qian Cai wrote: > > >=20 > > >=20 > > > > On Feb 8, 2020, at 8:44 PM, John Hubbard wr= ote: > > > >=20 > > > > So it looks like we're probably stuck with having to annotate the= code. Given > > > > that, there is a balance between how many macros, and how much co= mmenting. For > > > > example, if there is a single macro (data_race, for example), the= n we'll need to > > > > add comments for the various cases, explaining which data_race si= tuation is > > > > happening. > > >=20 > > > On the other hand, it is perfect fine of not commenting on each dat= a_race() that most of times, people could run git blame to learn more det= ails. Actually, no maintainers from various of subsystems asked for comme= nting so far. > > >=20 > >=20 > > Well, maybe I'm looking at this wrong. I was thinking that one should= attempt to > > understand the code on the screen, and that's generally best--but her= e, maybe > > "data_race" is just something that means "tool cruft", really. So men= tally we > > would move toward visually filtering out the data_race "key word". >=20 > One thing to note is that 'data_race()' points out concurrency, and > that somebody has deemed that the code won't break even with data > races. Somebody trying to understand or modify the code should ensure > this will still be the case. So, 'data_race()' isn't just tool cruft. > It's documentation for something that really isn't obvious from the > code alone. >=20 > Whenever we see a READ_ONCE or other marked access it is obvious to > the reader that there are concurrent accesses happening. I'd argue > that for intentional data races, we should convey similar information, > to avoid breaking the code (of course KCSAN would tell you, but only > after the change was done). Even moreso, since changes to code > involving 'data_race()' will need re-verification that the data races > are still safe. >=20 > > I really don't like it but at least there is a significant benefit fr= om the tool > > that probably makes it worth the visual noise. > >=20 > > Blue sky thoughts for The Far Future: It would be nice if the tools g= ot a lot > > better--maybe in the direction of C language extensions, even if only= used in > > this project at first. >=20 > Still thinking about this. What we want to convey is that, while > there are races on the particular variable, nobody should be modifying > the bits here. Adding a READ_ONCE (or data_race()) would miss a > harmful race where somebody modifies these bits, so in principle I > agree. However, I think the tool can't automatically tell (even if we > had compiler extensions to give us the bits accessed) which bits we > care about, because we might have something like: >=20 > int foo_bar =3D READ_ONCE(flags) >> FOO_BAR_SHIFT; // need the > READ_ONCE because of FOO bits > .. (foo_bar & FOO_MASK) .. // FOO bits can be modified concurrently > .. (foo_bar & BAR_MASK) .. // nobody should modify BAR bits > concurrently though ! >=20 > What we want is to assert that nobody touches a particular set of > bits. KCSAN has recently gotten ASSERT_EXCLUSIVE_{WRITER,ACCESS} > macros which help assert properties of concurrent code, where bugs > won't manifest as data races. Along those lines, I can see the value > in doing an exclusivity check on a bitmask of a variable. >=20 > I don't know how much a READ_BITS macro could help, since it's > probably less ergonomic to have to say something like: > READ_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT) >> ZONES_PGSHIFT. >=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 those > bits and safe w.r.t. data races. >=20 > For example, this may be used when certain bits of @flags 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 << ZONES_PGSHIFT)= ; > return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK; > } Actually, it seems still need to write if I understand correctly, ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT); return data_race((page->flags >> ZONES_PGSHIFT) & ZONES_MASK); On the other hand, if you really worry about this thing could go wrong, i= t might be better of using READ_ONCE() at the first place where it will be more f= uture- proof with the trade-off it might generate less efficient code optimizati= on? Alternatively, is there a way to write this as this? return ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT); Kind of ugly but it probably cleaner. >=20 > This will accomplish the following: > 1. The current code is not touched, and we do not have to verify that > the change is correct without KCSAN. > 2. We're not introducing a bunch of special macros to read bits in vari= ous 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 tell you > about the race. > 5. We're documenting the code. >=20 > Anything I missed? >=20 > Thanks, > -- Marco >=20 >=20 >=20 >=20 >=20 > > thanks, > > -- > > John Hubbard > > NVIDIA > >=20 > > > >=20 > > > > That's still true, but to a lesser extent if more macros are adde= d. In this case, > > > > I suspect that READ_BITS() makes the commenting easier and shorte= r. So I'd tentatively > > > > lead towards adding it, but what do others on the list think? > > >=20 > > > Even read bits could be dangerous from data races and confusing at = best, so I am not really sure what the value of introducing this new macr= o. People who like to understand it correctly still need to read the comm= it logs. > > >=20 > > > This flags->zonenum is such a special case that I don=E2=80=99t rea= lly see it regularly for the last few weeks digging KCSAN reports, so eve= n if it is worth adding READ_BITS(), there are more equally important mac= ros need to be added together to be useful initially. For example, HARMLE= SS_COUNTERS(), READ_SINGLE_BIT(), READ_IMMUTATABLE_BITS() etc which Linus= said exactly wanted to avoid. > > >=20