From: Marco Elver <elver@google.com> To: Bhupesh Sharma <bhsharma@redhat.com> Cc: akiyks@gmail.com, stern@rowland.harvard.edu, Alexander Potapenko <glider@google.com>, parri.andrea@gmail.com, andreyknvl@google.com, Andy Lutomirski <luto@kernel.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Arnd Bergmann <arnd@arndb.de>, boqun.feng@gmail.com, Borislav Petkov <bp@alien8.de>, dja@axtens.net, dlustig@nvidia.com, Dave Hansen <dave.hansen@linux.intel.com>, David Howells <dhowells@redhat.com>, Dmitry Vyukov <dvyukov@google.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, j.alglave@ucl.ac.uk, joel@joelfernandes.org, Jonathan Corbet <corbet@lwn.net>, Josh Poimboeuf <jpoimboe@redhat.com>, luc.maranget@inria.fr, Mark Rutland <mark.rutland@arm.com>, npiggin@gmail.com, paulmck@kernel.org, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Will Deacon <will@kernel.org>, kasan-dev@googlegroups.com, linux-arch@vger.kernel.org, Linux Doc Mailing List <linux-doc@vger.kernel.org>, linux-efi@vger.kernel.org, linux-kbuild@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-mm@kvack.org, x86@kernel.org Subject: Re: [PATCH v3 1/9] kcsan: Add Kernel Concurrency Sanitizer infrastructure Date: Fri, 8 Nov 2019 15:23:31 +0100 [thread overview] Message-ID: <20191108142331.GA201027@google.com> (raw) In-Reply-To: <CACi5LpMt1Jp3zi3dQXe-x=nZ4ikADoD2Sr4-6t4HKaarLs7uxw@mail.gmail.com> 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 <elver@google.com> 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 <linux/types.h> > > +#include <linux/kcsan-checks.h> > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Marco Elver <elver@google.com> To: Bhupesh Sharma <bhsharma@redhat.com> Cc: akiyks@gmail.com, stern@rowland.harvard.edu, Alexander Potapenko <glider@google.com>, parri.andrea@gmail.com, andreyknvl@google.com, Andy Lutomirski <luto@kernel.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Arnd Bergmann <arnd@arndb.de>, boqun.feng@gmail.com, Borislav Petkov <bp@alien8.de>, dja@axtens.net, dlustig@nvidia.com, Dave Hansen <dave.hansen@linux.intel.com>, David Howells <dhowells@redhat.com>, Dmitry Vyukov <dvyukov@google.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, j.alglave@ucl.ac.uk, joel@joelfernandes.org, Jonathan Corbet <corbet@lwn.net>, Josh Poimboeuf <jpoimboe@redhat.com>, luc.maranget@inria.fr, Mark Rutland <mark.rutland@arm.com>, npiggin@gmail.com, paulmck@kernel.org, Peter Zijlstra <peterz@infradead.org> Subject: Re: [PATCH v3 1/9] kcsan: Add Kernel Concurrency Sanitizer infrastructure Date: Fri, 8 Nov 2019 15:23:31 +0100 [thread overview] Message-ID: <20191108142331.GA201027@google.com> (raw) In-Reply-To: <CACi5LpMt1Jp3zi3dQXe-x=nZ4ikADoD2Sr4-6t4HKaarLs7uxw@mail.gmail.com> 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 <elver@google.com> 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 <linux/types.h> > > +#include <linux/kcsan-checks.h> > > 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
next prev parent reply other threads:[~2019-11-08 14:23 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-04 14:27 [PATCH v3 0/9] Add Kernel Concurrency Sanitizer (KCSAN) Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 1/9] kcsan: Add Kernel Concurrency Sanitizer infrastructure Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-06 9:38 ` Dmitry Vyukov 2019-11-06 9:38 ` Dmitry Vyukov 2019-11-06 9:38 ` Dmitry Vyukov 2019-11-06 10:03 ` Marco Elver 2019-11-06 10:03 ` Marco Elver 2019-11-06 10:03 ` Marco Elver 2019-11-06 19:11 ` Marco Elver 2019-11-06 19:11 ` Marco Elver 2019-11-06 19:59 ` kbuild test robot 2019-11-06 19:59 ` kbuild test robot 2019-11-06 19:59 ` kbuild test robot 2019-11-06 19:59 ` kbuild test robot 2019-11-06 20:34 ` kbuild test robot 2019-11-06 20:34 ` kbuild test robot 2019-11-06 20:34 ` kbuild test robot 2019-11-06 20:34 ` kbuild test robot 2019-11-07 18:43 ` Marco Elver 2019-11-07 18:43 ` Marco Elver 2019-11-07 18:43 ` Marco Elver 2019-11-07 18:43 ` Marco Elver 2019-11-07 21:08 ` Bhupesh Sharma 2019-11-07 21:08 ` Bhupesh Sharma 2019-11-07 21:08 ` Bhupesh Sharma 2019-11-08 14:23 ` Marco Elver [this message] 2019-11-08 14:23 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 2/9] kcsan: Add Documentation entry in dev-tools Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 3/9] objtool, kcsan: Add KCSAN runtime functions to whitelist Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 4/9] build, kcsan: Add KCSAN build exceptions Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 5/9] seqlock, kcsan: Add annotations for KCSAN Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-05 11:35 ` kbuild test robot 2019-11-05 11:35 ` kbuild test robot 2019-11-05 11:35 ` kbuild test robot 2019-11-05 11:35 ` kbuild test robot 2019-11-05 15:22 ` Marco Elver 2019-11-05 15:22 ` Marco Elver 2019-11-05 15:22 ` Marco Elver 2019-11-05 15:22 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 6/9] seqlock: Require WRITE_ONCE surrounding raw_seqcount_barrier Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 7/9] asm-generic, kcsan: Add KCSAN instrumentation for bitops Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 8/9] locking/atomics, kcsan: Add KCSAN instrumentation Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 14:27 ` [PATCH v3 9/9] x86, kcsan: Enable KCSAN for x86 Marco Elver 2019-11-04 14:27 ` Marco Elver 2019-11-04 16:47 ` [PATCH v3 0/9] Add Kernel Concurrency Sanitizer (KCSAN) Paul E. McKenney 2019-11-04 18:41 ` Marco Elver 2019-11-04 18:41 ` Marco Elver 2019-11-04 18:41 ` Marco Elver 2019-11-04 19:46 ` Paul E. McKenney 2019-11-04 19:46 ` Paul E. McKenney 2019-11-05 11:10 ` Marco Elver 2019-11-05 11:10 ` Marco Elver 2019-11-05 11:10 ` Marco Elver 2019-11-05 14:20 ` Paul E. McKenney 2019-11-05 14:20 ` Paul E. McKenney 2019-11-05 15:25 ` Marco Elver 2019-11-05 15:25 ` Marco Elver 2019-11-05 15:25 ` Marco Elver 2019-11-14 18:05 ` Marco Elver 2019-11-14 18:05 ` Marco Elver 2019-11-14 18:05 ` Marco Elver 2019-11-14 19:48 ` Paul E. McKenney 2019-11-14 19:48 ` Paul E. McKenney
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191108142331.GA201027@google.com \ --to=elver@google.com \ --cc=akiyks@gmail.com \ --cc=andreyknvl@google.com \ --cc=ard.biesheuvel@linaro.org \ --cc=arnd@arndb.de \ --cc=bhsharma@redhat.com \ --cc=boqun.feng@gmail.com \ --cc=bp@alien8.de \ --cc=corbet@lwn.net \ --cc=dave.hansen@linux.intel.com \ --cc=dhowells@redhat.com \ --cc=dja@axtens.net \ --cc=dlustig@nvidia.com \ --cc=dvyukov@google.com \ --cc=glider@google.com \ --cc=hpa@zytor.com \ --cc=j.alglave@ucl.ac.uk \ --cc=joel@joelfernandes.org \ --cc=jpoimboe@redhat.com \ --cc=kasan-dev@googlegroups.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kbuild@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=luc.maranget@inria.fr \ --cc=luto@kernel.org \ --cc=mark.rutland@arm.com \ --cc=mingo@redhat.com \ --cc=npiggin@gmail.com \ --cc=parri.andrea@gmail.com \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=stern@rowland.harvard.edu \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.