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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A424DC433EF for ; Mon, 6 Dec 2021 14:33:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2BF4C6B006C; Mon, 6 Dec 2021 09:32:59 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 247C46B007D; Mon, 6 Dec 2021 09:32:59 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 073F96B007E; Mon, 6 Dec 2021 09:32:59 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0161.hostedemail.com [216.40.44.161]) by kanga.kvack.org (Postfix) with ESMTP id EA9BE6B006C for ; Mon, 6 Dec 2021 09:32:58 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id A55C0184558EB for ; Mon, 6 Dec 2021 14:32:48 +0000 (UTC) X-FDA: 78887610816.29.CDFC8FF Received: from mail-io1-f43.google.com (mail-io1-f43.google.com [209.85.166.43]) by imf23.hostedemail.com (Postfix) with ESMTP id 45143900009B for ; Mon, 6 Dec 2021 14:32:48 +0000 (UTC) Received: by mail-io1-f43.google.com with SMTP id e128so13150016iof.1 for ; Mon, 06 Dec 2021 06:32:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=FHezbGrYlKxA1aTrkxNXSkuk1RduKCiWsmFP87kNHNY=; b=GXfZLWErwVXNUIWTHzJ3JSvvaZ1ekUxwLx/RO6eG2oqG8RKwA83QEHyf6Lgu2pvH2e 6EAZXpfRzfvbjY71KDHAzWFUD1D5xnfkxDwd1DAzcCl83aTQm6Ne32mTBtwoQgT1CHUV 2W8cDfYJZTbjYPvaVlm+oBaInxz7hnYHaJtrKCW3ZUfFjBzMZQu8pSL7cZgSKpwByPwK gdOYhET5KSaY7yVXH/pmeLnQFr/R51Qxn+A8CxvMJ/5bQhb/CoYCURhE7gJQplzFUgS0 HnBwT1uFanfIiFq2JoHW276Mrtt8WTSgmVf7U/BsIzKfQ9A9GkpI3mSaOpkBHdDcbQTZ Mhyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=FHezbGrYlKxA1aTrkxNXSkuk1RduKCiWsmFP87kNHNY=; b=0ESKZRgopxBcWEDMuHIG/+msr1YzkgKOny82B38+V8wWFAitf5TebN7htjG6Qo0ghu R8o5Fv3FxLr0gkgWGMT0Rbu9fhFZNzquVit1tyyBTCmcNqvCSz0KTSlldIy0xSBsZ1Ic 0PUfMCJlD4wr9pseObunDL/2MGvMWZRKR9YGeJ6M100UzG/wA2aw7kq+cJi7FeUrRXbt MK7m583pvAc9qvX4hiam41/FEAOXkUEiOVySyvEn+ep5qYa7l6UcqOomsP05cyieNkH9 5WvAHPBlyy31LW5i3DiWv+K22FWnGakoElWPk5gybg/2daHKXXWSiNCMxcrrVDfg9mzY hlvQ== X-Gm-Message-State: AOAM532ZkfRQtBO0Wgnd9EsZAcEBHWUhc4fTJaB9k2owr3Ob4WLl7vJt MBMmLUrKM9hBvltqs5Dk1fk= X-Google-Smtp-Source: ABdhPJx2s79y4UV6UfCELoWwjsBXrn2U9Tro1XMfsLGc7m27xn/DOl5d0HW/E8jg1uMWElErsFJE7A== X-Received: by 2002:a02:a708:: with SMTP id k8mr42965751jam.26.1638801167426; Mon, 06 Dec 2021 06:32:47 -0800 (PST) Received: from auth1-smtp.messagingengine.com (auth1-smtp.messagingengine.com. [66.111.4.227]) by smtp.gmail.com with ESMTPSA id y8sm6337176iox.32.2021.12.06.06.32.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Dec 2021 06:32:46 -0800 (PST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailauth.nyi.internal (Postfix) with ESMTP id 8E52D27C0054; Mon, 6 Dec 2021 09:32:44 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Mon, 06 Dec 2021 09:32:44 -0500 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrjeefgdeihecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesthdtredttddtvdenucfhrhhomhepuehoqhhunhcu hfgvnhhguceosghoqhhunhdrfhgvnhhgsehgmhgrihhlrdgtohhmqeenucggtffrrghtth gvrhhnpeevieejtdfhieejfeduheehvdevgedugeethefggfdtvdeutdevgeetvddvfeeg tdenucffohhmrghinhepkhgvrhhnvghlrdhorhhgnecuvehluhhsthgvrhfuihiivgeptd enucfrrghrrghmpehmrghilhhfrhhomhepsghoqhhunhdomhgvshhmthhprghuthhhphgv rhhsohhnrghlihhthidqieelvdeghedtieegqddujeejkeehheehvddqsghoqhhunhdrfh gvnhhgpeepghhmrghilhdrtghomhesfhhigihmvgdrnhgrmhgv X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 6 Dec 2021 09:32:43 -0500 (EST) Date: Mon, 6 Dec 2021 22:31:24 +0800 From: Boqun Feng To: Marco Elver Cc: "Paul E. McKenney" , Alexander Potapenko , Borislav Petkov , Dmitry Vyukov , Ingo Molnar , Mark Rutland , Peter Zijlstra , Thomas Gleixner , Waiman Long , Will Deacon , kasan-dev@googlegroups.com, linux-arch@vger.kernel.org, linux-doc@vger.kernel.org, linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, llvm@lists.linux.dev, x86@kernel.org Subject: Re: [PATCH v3 08/25] kcsan: Show location access was reordered to Message-ID: References: <20211130114433.2580590-1-elver@google.com> <20211130114433.2580590-9-elver@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 45143900009B X-Stat-Signature: 8jow4mhq33ko5ai5op318wysinb7z51k Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=GXfZLWEr; spf=pass (imf23.hostedemail.com: domain of boqun.feng@gmail.com designates 209.85.166.43 as permitted sender) smtp.mailfrom=boqun.feng@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-HE-Tag: 1638801168-551851 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, Dec 06, 2021 at 08:16:11AM +0100, Marco Elver wrote: > On Mon, 6 Dec 2021 at 06:04, Boqun Feng wrote: > > > > Hi, > > > > On Tue, Nov 30, 2021 at 12:44:16PM +0100, Marco Elver wrote: > > > Also show the location the access was reordered to. An example report: > > > > > > | ================================================================== > > > | BUG: KCSAN: data-race in test_kernel_wrong_memorder / test_kernel_wrong_memorder > > > | > > > | read-write to 0xffffffffc01e61a8 of 8 bytes by task 2311 on cpu 5: > > > | test_kernel_wrong_memorder+0x57/0x90 > > > | access_thread+0x99/0xe0 > > > | kthread+0x2ba/0x2f0 > > > | ret_from_fork+0x22/0x30 > > > | > > > | read-write (reordered) to 0xffffffffc01e61a8 of 8 bytes by task 2310 on cpu 7: > > > | test_kernel_wrong_memorder+0x57/0x90 > > > | access_thread+0x99/0xe0 > > > | kthread+0x2ba/0x2f0 > > > | ret_from_fork+0x22/0x30 > > > | | > > > | +-> reordered to: test_kernel_wrong_memorder+0x80/0x90 > > > | > > > > Should this be "reordered from" instead of "reordered to"? For example, > > if the following case needs a smp_mb() between write to A and write to > > B, I think currently it will report as follow: > > > > foo() { > > WRITE_ONCE(A, 1); // let's say A's address is 0xaaaa > > bar() { > > WRITE_ONCE(B, 1); // Assume B's address is 0xbbbb > > // KCSAN find the problem here > > } > > } > > > > > > | write (reordered) to 0xaaaa of ...: > > | bar+0x... // address of the write to B > > | foo+0x... // address of the callsite to bar() > > | ... > > | | > > | +-> reordered to: foo+0x... // address of the write to A > > > > But since the access reported here is the write to A, so it's a > > "reordered from" instead of "reordered to"? > > Perhaps I could have commented on this in the commit message to avoid > the confusion, but per its updated comment replace_stack_entry() > "skips to the first entry that matches the function of @ip, and then > replaces that entry with @ip, returning the entries to skip with > @replaced containing the replaced entry." > > When a reorder_access is set up, the ip to it is stored, which is > what's passed to @ip of replace_stack_entry(). It effectively swaps > the top frame where the race occurred with where the original access > happened. This all works because the runtime is careful to only keep > reorder_accesses valid until the original function where it occurred > is left. > Thanks for the explanation, I was missing the swap here. However... > So in your above example you need to swap "reordered to" and the top > frame of the stack trace. > IIUC, the report for my above example will be: | write (reordered) to 0xaaaa of ...: | foo+0x... // address of the write to A | ... | | | +-> reordered to: foo+0x... // address of the callsite to bar() in foo() , right? Because in replace_stack_entry(), it's not the top frame where the race occurred that gets swapped, it's the frame which belongs to the same function as the original access that gets swapped. In other words, when KCSAN finds the problem, top entries of the calling stack are: [0] bar+0x.. // address of the write to B [1] foo+0x.. // address of the callsite to bar() in foo() after replace_stack_entry(), they changes to: [0] bar+0x.. // address of the write to B skip ->[1] foo+0x.. // address of the write to A , as a result the report won't mention bar() at all. And I think a better report will be: | write (reordered) to 0xaaaa of ...: | foo+0x... // address of the write to A | ... | | | +-> reordered to: bar+0x... // address of the write to B in bar() because it tells users the exact place the accesses get reordered. That means maybe we want something as below? Not completely tested, but I play with scope checking a bit, seems it gives what I want. Thoughts? Regards, Boqun diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 67794404042a..b495ed3aa637 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -324,7 +324,10 @@ replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned lon else goto fallback; - for (skip = 0; skip < num_entries; ++skip) { + skip = get_stack_skipnr(stack_entries, num_entries); + *replaced = stack_entries[skip]; + + for (;skip < num_entries; ++skip) { unsigned long func = stack_entries[skip]; if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset)) @@ -332,7 +335,6 @@ replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned lon func -= offset; if (func == target_func) { - *replaced = stack_entries[skip]; stack_entries[skip] = ip; return skip; } > The implementation is a little trickier of course, but I really wanted > the main stack trace to look like any other non-reordered access, > which starts from the original access, and only have the "reordered > to" location be secondary information. > > The foundation for doing this this was put in place here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6c65eb75686f > > Thanks, > -- Marco