From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marco Elver Subject: Re: [PATCH v3 1/9] kcsan: Add Kernel Concurrency Sanitizer infrastructure Date: Fri, 8 Nov 2019 15:23:31 +0100 Message-ID: <20191108142331.GA201027@google.com> References: <20191104142745.14722-1-elver@google.com> <20191104142745.14722-2-elver@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Bhupesh Sharma Cc: akiyks@gmail.com, stern@rowland.harvard.edu, Alexander Potapenko , parri.andrea@gmail.com, andreyknvl@google.com, Andy Lutomirski , Ard Biesheuvel , Arnd Bergmann , boqun.feng@gmail.com, Borislav Petkov , dja@axtens.net, dlustig@nvidia.com, Dave Hansen , David Howells , Dmitry Vyukov , "H. Peter Anvin" , Ingo Molnar , j.alglave@ucl.ac.uk, joel@joelfernandes.org, Jonathan Corbet , Josh Poimboeuf , luc.maranget@inria.fr, Mark Rutland , npiggin@gmail.com, paulmck@kernel.org, Peter Zijlstra List-Id: linux-arch.vger.kernel.org Hi Bhupesh, Thanks for your comments, see answers below. On Fri, 08 Nov 2019, Bhupesh Sharma wrote: > Sorry for the late comments, but I am just trying to understand the > new KCSAN feature (which IMO seems very useful for debugging issues). > > Some comments inline: > > On Mon, Nov 4, 2019 at 7:59 PM Marco Elver wrote: > > ... > > diff --git a/include/linux/kcsan.h b/include/linux/kcsan.h > > new file mode 100644 > > index 000000000000..bd8122acae01 > > --- /dev/null > > +++ b/include/linux/kcsan.h > > @@ -0,0 +1,115 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#ifndef _LINUX_KCSAN_H > > +#define _LINUX_KCSAN_H > > + > > +#include > > +#include > > For the new changes introduced (especially the new header files), can > we please try to keep the alphabetical order > for the include'd files. > > The same comment applies for changes below ... Done for v4. ... > > +void kcsan_disable_current(void) > > +{ > > + ++get_ctx()->disable_count; > > +} > > +EXPORT_SYMBOL(kcsan_disable_current); > > + > > +void kcsan_enable_current(void) > > +{ > > + if (get_ctx()->disable_count-- == 0) { > > + kcsan_disable_current(); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > I am not sure I understand, why we need to call > 'kcsan_disable_current()' twice and what the WARN message conveys. > May-be you can add a comment here, or a more descriptive WARN meesage. This branch is entered when there is an imbalance between kcsan_disable_current and kcsan_enable_current calls. When entering the branch, the decrement transitioned disable_count to -1, which should not happen. The call to kcsan_disable_current restores it to 0, and the following kcsan_disable_current actually disables KCSAN for generating the warning. > > + kcsan_enable_current(); > > + } > > +} > > +EXPORT_SYMBOL(kcsan_enable_current); > > + > > +void kcsan_nestable_atomic_begin(void) > > +{ > > + /* > > + * Do *not* check and warn if we are in a flat atomic region: nestable > > + * and flat atomic regions are independent from each other. > > + * See include/linux/kcsan.h: struct kcsan_ctx comments for more > > + * comments. > > + */ > > + > > + ++get_ctx()->atomic_nest_count; > > +} > > +EXPORT_SYMBOL(kcsan_nestable_atomic_begin); > > + > > +void kcsan_nestable_atomic_end(void) > > +{ > > + if (get_ctx()->atomic_nest_count-- == 0) { > > + kcsan_nestable_atomic_begin(); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > .. Same as above. Same situation, except for atomic_nest_count. Here also atomic_nest_count is -1 which should not happen. I've added some more comments. > > + kcsan_enable_current(); > > + } > > +} > > +EXPORT_SYMBOL(kcsan_nestable_atomic_end); Best Wishes, -- Marco From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com ([209.85.221.68]:40935 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726485AbfKHOXm (ORCPT ); Fri, 8 Nov 2019 09:23:42 -0500 Received: by mail-wr1-f68.google.com with SMTP id i10so7247618wrs.7 for ; Fri, 08 Nov 2019 06:23:39 -0800 (PST) Date: Fri, 8 Nov 2019 15:23:31 +0100 From: Marco Elver Subject: Re: [PATCH v3 1/9] kcsan: Add Kernel Concurrency Sanitizer infrastructure Message-ID: <20191108142331.GA201027@google.com> References: <20191104142745.14722-1-elver@google.com> <20191104142745.14722-2-elver@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Bhupesh Sharma Cc: akiyks@gmail.com, stern@rowland.harvard.edu, Alexander Potapenko , parri.andrea@gmail.com, andreyknvl@google.com, Andy Lutomirski , Ard Biesheuvel , Arnd Bergmann , boqun.feng@gmail.com, Borislav Petkov , dja@axtens.net, dlustig@nvidia.com, Dave Hansen , David Howells , Dmitry Vyukov , "H. Peter Anvin" , Ingo Molnar , j.alglave@ucl.ac.uk, joel@joelfernandes.org, Jonathan Corbet , Josh Poimboeuf , luc.maranget@inria.fr, Mark Rutland , npiggin@gmail.com, paulmck@kernel.org, Peter Zijlstra , Thomas Gleixner , Will Deacon , kasan-dev@googlegroups.com, linux-arch@vger.kernel.org, Linux Doc Mailing List , linux-efi@vger.kernel.org, linux-kbuild@vger.kernel.org, Linux Kernel Mailing List , linux-mm@kvack.org, x86@kernel.org Message-ID: <20191108142331.amoyq7aiBcKpoYD3Ym5kWojhAmsD6qxNjh1ngCcqpkE@z> Hi Bhupesh, Thanks for your comments, see answers below. On Fri, 08 Nov 2019, Bhupesh Sharma wrote: > Sorry for the late comments, but I am just trying to understand the > new KCSAN feature (which IMO seems very useful for debugging issues). > > Some comments inline: > > On Mon, Nov 4, 2019 at 7:59 PM Marco Elver wrote: > > ... > > diff --git a/include/linux/kcsan.h b/include/linux/kcsan.h > > new file mode 100644 > > index 000000000000..bd8122acae01 > > --- /dev/null > > +++ b/include/linux/kcsan.h > > @@ -0,0 +1,115 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#ifndef _LINUX_KCSAN_H > > +#define _LINUX_KCSAN_H > > + > > +#include > > +#include > > For the new changes introduced (especially the new header files), can > we please try to keep the alphabetical order > for the include'd files. > > The same comment applies for changes below ... Done for v4. ... > > +void kcsan_disable_current(void) > > +{ > > + ++get_ctx()->disable_count; > > +} > > +EXPORT_SYMBOL(kcsan_disable_current); > > + > > +void kcsan_enable_current(void) > > +{ > > + if (get_ctx()->disable_count-- == 0) { > > + kcsan_disable_current(); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > I am not sure I understand, why we need to call > 'kcsan_disable_current()' twice and what the WARN message conveys. > May-be you can add a comment here, or a more descriptive WARN meesage. This branch is entered when there is an imbalance between kcsan_disable_current and kcsan_enable_current calls. When entering the branch, the decrement transitioned disable_count to -1, which should not happen. The call to kcsan_disable_current restores it to 0, and the following kcsan_disable_current actually disables KCSAN for generating the warning. > > + kcsan_enable_current(); > > + } > > +} > > +EXPORT_SYMBOL(kcsan_enable_current); > > + > > +void kcsan_nestable_atomic_begin(void) > > +{ > > + /* > > + * Do *not* check and warn if we are in a flat atomic region: nestable > > + * and flat atomic regions are independent from each other. > > + * See include/linux/kcsan.h: struct kcsan_ctx comments for more > > + * comments. > > + */ > > + > > + ++get_ctx()->atomic_nest_count; > > +} > > +EXPORT_SYMBOL(kcsan_nestable_atomic_begin); > > + > > +void kcsan_nestable_atomic_end(void) > > +{ > > + if (get_ctx()->atomic_nest_count-- == 0) { > > + kcsan_nestable_atomic_begin(); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > .. Same as above. Same situation, except for atomic_nest_count. Here also atomic_nest_count is -1 which should not happen. I've added some more comments. > > + kcsan_enable_current(); > > + } > > +} > > +EXPORT_SYMBOL(kcsan_nestable_atomic_end); Best Wishes, -- Marco