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.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 DEF29C2BA16 for ; Wed, 8 Apr 2020 22:44:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 962612078E for ; Wed, 8 Apr 2020 22:44:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="W6ByfA+f" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 962612078E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 20E4C8E000D; Wed, 8 Apr 2020 18:44:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1992A8E0006; Wed, 8 Apr 2020 18:44:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0620D8E000D; Wed, 8 Apr 2020 18:44:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0001.hostedemail.com [216.40.44.1]) by kanga.kvack.org (Postfix) with ESMTP id DC04E8E0006 for ; Wed, 8 Apr 2020 18:44:39 -0400 (EDT) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 8EC3D98B0 for ; Wed, 8 Apr 2020 22:44:39 +0000 (UTC) X-FDA: 76686168678.02.ocean85_7feefbb2c4e1c X-HE-Tag: ocean85_7feefbb2c4e1c X-Filterd-Recvd-Size: 6938 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf24.hostedemail.com (Postfix) with ESMTP for ; Wed, 8 Apr 2020 22:44:39 +0000 (UTC) Received: from paulmck-ThinkPad-P72.home (50-39-105-78.bvtn.or.frontiernet.net [50.39.105.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3FE5C20730; Wed, 8 Apr 2020 22:44:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1586385878; bh=/GnrN3yMhGvFNsv2SWgRpkM+pZY9kGf1n7T0e4pYEj8=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=W6ByfA+fJPdUClhSVfw2/6wxfooL1pCJ8uLCmrKr/wiAhyD+eqmTWb/YRf+fpvH+Q BE1LDvJNGk8ocF1DQHjlI28VMZ743t/o42RWmlEyL3sAya2BAZaKkuzlsyqCXjyyna AEIivhMWAVQLeN9boW+2av7gAKozpCkn6xzSg0AQ= Received: by paulmck-ThinkPad-P72.home (Postfix, from userid 1000) id 14B233523234; Wed, 8 Apr 2020 15:44:38 -0700 (PDT) Date: Wed, 8 Apr 2020 15:44:38 -0700 From: "Paul E. McKenney" To: Chris Wilson Cc: Andrew Morton , Chris Wilson , David Laight , Marco Elver , Linux-MM , Mark Rutland , mm-commits@vger.kernel.org, Randy Dunlap , stable Subject: Re: [patch 125/166] lib/list: prevent compiler reloads inside 'safe' list iteration Message-ID: <20200408224438.GA4654@paulmck-ThinkPad-P72> Reply-To: paulmck@kernel.org References: <20200406200254.a69ebd9e08c4074e41ddebaf@linux-foundation.org> <20200407031042.8o-fYMox-%akpm@linux-foundation.org> <20200408202630.GA1666@paulmck-ThinkPad-P72> <20200408223757.GG17661@paulmck-ThinkPad-P72> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200408223757.GG17661@paulmck-ThinkPad-P72> User-Agent: Mutt/1.9.4 (2018-02-28) 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, Apr 08, 2020 at 03:37:57PM -0700, Paul E. McKenney wrote: > On Wed, Apr 08, 2020 at 02:14:38PM -0700, Linus Torvalds wrote: > > On Wed, Apr 8, 2020 at 1:26 PM Paul E. McKenney wrote: > > > > > > OK, it sounds like we need to specify what needs to be present in this > > > sort of commit log. > > > > That would have helped. > > > > But in this case, after having looked more at it, it wasn't that the > > commit log was not sufficient, it was that the change was wrong. > > > > With a better commit log and a pointer to the KCSAN report, I would > > have been able to make a better judgment call earlier, and not have > > had to ask for more information. > > > > But once I figured out what the problem was, it was clear that > > > > (a) what the i915 driver does is at a minimum questionable, and quite > > likely actively buggy > > > > (b) even if it's not buggy in the i915 driver, changing the list > > traversal macros to use READ_ONCE() would hide other places where it > > most definitely is buggy. > > Yeah, and I should especially have spotted this last one, given how many > times I have run into it while using KCSAN within RCU. ;-/ > > In any case, we have review comments, discussion, and the occasional > controvery for non-KCSAN bug fixes, so it is only reasonable to expect > a similar process for KCSAN bug reports, right? My main goal here is > to make things easier for people reviewing future KCSAN-inspired fixes. > > > > o The KCSAN splat, optionally including file/line numbers. > > > > I do think that the KCSAN splat - very preferably simplified and with > > explanations - should basically always be included if KCSAN was the > > reason. > > > > Of course, the "simplified and with explanations" may be simplified to > > the point where none of the original splat even remains. Those splats > > aren't so legible that anybody should feel like they should be > > included. > > > > Put another way: an analysis that is so thorough that it makes the raw > > splat data pointless is a *good* thing, and if that means that no > > splat is needed, all the better. > > Fair point. The KCSAN splat has the added advantage of immediately > answering the "but does this really happen?" question, but then again > that question could also be answered by just stating that the issue was > found using KCSAN. > > > > o Detailed description of the problem this splat identifies, for > > > example, the code might fail to acquire a necessary lock, a plain > > > load might vulnerable to compiler optimizations, and so on. > > > > See above: if the description is detailed enough, then the splat > > itself becomes much less interesting. > > > > But in this case, for example, it's worth pointing out that the "safe" > > list accessors are used all over the kernel, and they are very much > > _not_ thread-safe. The "safe" in them is purely about the current > > thread removing an entry, not some kind of general thread-safeness. > > > > Which is why I decided I really hated that patch - it basically would > > make KCSAN unable to find _real_ races elsewhere, because it hid a > > very special race in the i915 driver. > > > > So I started out with the "that can't be right" kind of general > > feeling of uneasiness about the patch. I ended up with a much more > > specific "no, that's very much not right" and no amount of commit log > > should have saved it. > > > > As mentioned, I suspect that the i915 driver could actually use the > > RCU-safe list walkers. Their particular use is not about RCU per se, > > but it has some very similar rules about walking a list concurrently > > with somebody adding an entry to it. > > > > And the RCU list walkers do end up doing special things to load the > > next pointer. > > > > Whether we want to say - and document - that "ok, the RCU list walkers > > are also useful for this non-RCU use case" in general might be worth > > discussing. This might work quite well, given the lockdep annotations that Joel added to list_for_each_entry_rcu(). For example, lockdep_assert_held() would tell it that it didn't need to be in an RCU reader as long as the lock passed to lockdep_assert_held() was held at that point. > > It may be that the i915 use case is so special that it's only worth > > documenting there. > > If documenting is the right approach, KCSAN's data_race() could be > thought of as KCSAN-visible documentation. > > I will touch base with Chris separately. Of these options, what looks best to you? Or would something else work better? Thanx, Paul