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=-8.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 C032BC352A3 for ; Mon, 10 Feb 2020 16:34:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 739C02080C for ; Mon, 10 Feb 2020 16:34:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="HmUZAl2b" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 739C02080C 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 E377A6B012A; Mon, 10 Feb 2020 11:34:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DE7986B012D; Mon, 10 Feb 2020 11:34:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CD6D46B012E; Mon, 10 Feb 2020 11:34:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0252.hostedemail.com [216.40.44.252]) by kanga.kvack.org (Postfix) with ESMTP id B19F66B012A for ; Mon, 10 Feb 2020 11:34:01 -0500 (EST) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4A2AF824934B for ; Mon, 10 Feb 2020 16:34:01 +0000 (UTC) X-FDA: 76474764282.24.lead17_402251dda301c X-HE-Tag: lead17_402251dda301c X-Filterd-Recvd-Size: 11047 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf14.hostedemail.com (Postfix) with ESMTP for ; Mon, 10 Feb 2020 16:34:00 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id j20so6952748otq.3 for ; Mon, 10 Feb 2020 08:34:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=wf+AhbPiSiYAxsrHR8SQ2ICmgdSGnvtwAFNxR7pXl/o=; b=HmUZAl2bsyq77+pXYmX10yeTgx+tRYh5CVQNzljm47PnvGQgZCGT0Ivctd9JoWGKd7 6xalsQ9z+C2u3OCc5YEDYSLZQ162STAhyxZNbFKktwlOujKIYN50JwoP9a3ZRYK5UPe3 SqXgfrSaR24qXWHIsWRONbEGgcSSTobhQiZIyn04UCpE66la/GqkXG17dd0ew7DXCvSY GRI2J9P7bdQQhHM5qFdEm96ZODta2OAosPo95/Ggz+iOxaVlii9tqF2wksVhSVqoqc2U 04GLMST9ieIzpQiCcxxfsli5uU/2Ybd9IwlB9/oQy8DiyNDVdEtfdfXYw9qHfg/KzrjT fHmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=wf+AhbPiSiYAxsrHR8SQ2ICmgdSGnvtwAFNxR7pXl/o=; b=TblF5+Yut6RG+XfCVFJAyMjTBehs0sBFhqWYYjK2B/GAUPxjgfTU4nIClu8c60lo/k 3vi7wfnrRkSQw7bmxlLN8CVsXetoRirOEoTphm3+ZejfrqBTlNZF99Z/UGBTlElMD/2I WCkqS7lmi8KcRTT4UBnyudQFCCRJJS+p61JEUbPJPYzASW/onQr/XXccVc1c002XmFvf Qa6A1TqM/i5lAZDmkxus5w+KtLfbqwpnsTcT4TjXtAclg97xXsPgngCxsjtTcMo6ZVZT QaIWttONYRr5kLPkEwhuo426ZSNsDX+lnwSV2wx7j0Ok3gphfwYwSr+VkQi24Kmepu8M 5g4g== X-Gm-Message-State: APjAAAWbPZseCgZxew3BeIKencQtC7X9GowW55gK7smxryi14jtwuBNx 7/f1R3hmoenhc/FarvPo1Kl6MsZB09D8eyKbJhOnYg== X-Google-Smtp-Source: APXvYqzG7SAKQXagsbCW1ReGU/cYjpCT3aROpkINFf9MnnRb097PoIRclk3WJ4wfnaD2dmgy46Mtc6J96srAqoz4InM= X-Received: by 2002:a9d:7f12:: with SMTP id j18mr1781390otq.17.1581352439599; Mon, 10 Feb 2020 08:33:59 -0800 (PST) MIME-Version: 1.0 References: <5402183a-2372-b442-84d3-c28fb59fa7af@nvidia.com> <8602A57D-B420-489C-89CC-23D096014C47@lca.pw> <1a179bea-fd71-7b53-34c5-895986c24931@nvidia.com> <1581351789.7365.32.camel@lca.pw> In-Reply-To: <1581351789.7365.32.camel@lca.pw> From: Marco Elver Date: Mon, 10 Feb 2020 17:33:48 +0100 Message-ID: Subject: Re: [PATCH] mm: fix a data race in put_page() To: Qian Cai 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 Content-Type: text/plain; charset="UTF-8" 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, 10 Feb 2020 at 17:23, Qian Cai wrote: > > On Mon, 2020-02-10 at 08:48 +0100, Marco Elver wrote: > > On Sun, 9 Feb 2020 at 08:15, John Hubbard wrote: > > > > > > On 2/8/20 7:10 PM, Qian Cai wrote: > > > > > > > > > > > > > On Feb 8, 2020, at 8:44 PM, John Hubbard wr= ote: > > > > > > > > > > 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. > > > > > > > > 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 detai= ls. Actually, no maintainers from various of subsystems asked for commentin= g so far. > > > > > > > > > > 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". > > > > 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. > > > > 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. > > > > > 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. > > > > > > 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. > > > > 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: > > > > 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 ! > > > > 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. > > > > 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. > > > > Here is an alternative: > > > > 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. > > > > 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) .... > > > > Then we can write page_zonenum as follows: > > > > 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); No, I designed it so you won't need 'data_race()' if you don't want to. I'll send the patches shortly. > 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? The READ_ONCE() I'd still advocate for, but KCSAN won't complain if the pattern is as written above. > Alternatively, is there a way to write this as this? > > return ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT); It's an ASSERT, without KCSAN it should do nothing, so this is wrong. Also, this won't work because you're no longer returning the same value. I thought about this for READ_BITS, but you'd need (I wrote this earlier in the thread that it likely won't be suitable): READ_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT) >> ZONES_PGSHIFT to get the equivalent result (notice this will result in a redundant shift). Because we have all kinds of permutations and variants of how to extract the same bits out of some flags, it's cleaner to have one 'ASSERT_EXCLUSIVE_BITS' and just give it the bits you care about. Thanks, -- Marco > Kind of ugly but it probably cleaner. > > > > > 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. > > > > Anything I missed? > > > > Thanks, > > -- Marco > > > > > > > > > > > > > thanks, > > > -- > > > John Hubbard > > > NVIDIA > > > > > > > > > > > > > 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? > > > > > > > > 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 macro.= People who like to understand it correctly still need to read the commit l= ogs. > > > > > > > > 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 even = if it is worth adding READ_BITS(), there are more equally important macros = need to be added together to be useful initially. For example, HARMLESS_COU= NTERS(), READ_SINGLE_BIT(), READ_IMMUTATABLE_BITS() etc which Linus said ex= actly wanted to avoid. > > > >