From: Marco Elver <elver@google.com> To: Mark Rutland <mark.rutland@arm.com> Cc: LKMM Maintainers -- Akira Yokosawa <akiyks@gmail.com>, Alan Stern <stern@rowland.harvard.edu>, Alexander Potapenko <glider@google.com>, Andrea Parri <parri.andrea@gmail.com>, Andrey Konovalov <andreyknvl@google.com>, Andy Lutomirski <luto@kernel.org>, ard.biesheuvel@linaro.org, Arnd Bergmann <arnd@arndb.de>, Boqun Feng <boqun.feng@gmail.com>, Borislav Petkov <bp@alien8.de>, Daniel Axtens <dja@axtens.net>, Daniel Lustig <dlustig@nvidia.com>, dave.hansen@linux.intel.com, dhowells@redhat.com, Dmitry Vyukov <dvyukov@google.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, Jade Alglave <j.alglave@ucl.ac.uk>, Joel Fernandes <joel@joelfernandes.org>, Jonathan Corbet <corbet@lwn.net>, Josh Poimboeuf <jpoimboe@redhat.com>, Luc Maranget <luc.maranget@inria.fr>, Nicholas Piggin <npiggin@gmail.com>, "Paul E. McKenney" <paulmck@linux.ibm.com>, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Will Deacon <will@kernel.org>, kasan-dev <kasan-dev@googlegroups.com>, linux-arch <linux-arch@vger.kernel.org>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, linux-efi@vger.kernel.org, linux-kbuild@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, Linux Memory Management List <linux-mm@kvack.org>, "the arch/x86 maintainers" <x86@kernel.org> Subject: Re: [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure Date: Wed, 16 Oct 2019 17:53:45 +0200 [thread overview] Message-ID: <CANpmjNNctoVsUc+VbJ_RAMgLxcbvjq55gK1NdE0G0muMdv1+Ng@mail.gmail.com> (raw) In-Reply-To: <20191016151643.GC46264@lakrids.cambridge.arm.com> On Wed, 16 Oct 2019 at 17:16, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Oct 16, 2019 at 10:39:52AM +0200, Marco Elver wrote: > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 2c2e56bd8913..34a1d9310304 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -1171,6 +1171,13 @@ struct task_struct { > > #ifdef CONFIG_KASAN > > unsigned int kasan_depth; > > #endif > > +#ifdef CONFIG_KCSAN > > + /* See comments at kernel/kcsan/core.c: struct cpu_state. */ > > + int kcsan_disable; > > + int kcsan_atomic_next; > > + int kcsan_atomic_region; > > + bool kcsan_atomic_region_flat; > > +#endif > > Should these be unsigned? I prefer to keep them int, as they can become negative (rather than underflow with unsigned), if we e.g. have unbalanced kcsan_enable_current etc. Since we do not need the full unsigned range (these values should stay relatively small), int is more than enough. > > +/* > > + * Per-CPU state that should be used instead of 'current' if we are not in a > > + * task. > > + */ > > +struct cpu_state { > > + int disable; /* disable counter */ > > + int atomic_next; /* number of following atomic ops */ > > + > > + /* > > + * We use separate variables to store if we are in a nestable or flat > > + * atomic region. This helps make sure that an atomic region with > > + * nesting support is not suddenly aborted when a flat region is > > + * contained within. Effectively this allows supporting nesting flat > > + * atomic regions within an outer nestable atomic region. Support for > > + * this is required as there are cases where a seqlock reader critical > > + * section (flat atomic region) is contained within a seqlock writer > > + * critical section (nestable atomic region), and the "mismatching > > + * kcsan_end_atomic()" warning would trigger otherwise. > > + */ > > + int atomic_region; > > + bool atomic_region_flat; > > +}; > > +static DEFINE_PER_CPU(struct cpu_state, this_state) = { > > + .disable = 0, > > + .atomic_next = 0, > > + .atomic_region = 0, > > + .atomic_region_flat = 0, > > +}; > > These are the same as in task_struct, so I think it probably makes sense > to have a common structure for these, e.g. > > | struct kcsan_ctx { > | int disable; > | int atomic_next; > | int atomic_region; > | bool atomic_region_flat; > | }; > > ... which you then place within task_struct, e.g. > > | #ifdef CONFIG_KCSAN > | struct kcsan_ctx kcsan_ctx; > | #endif > > ... and here, e.g. > > | static DEFINE_PER_CPU(struct kcsan_ctx, kcsan_cpu_ctx); > > That would simplify a number of cases below where you have to choose one > or the other, as you can choose the pointer, then handle the rest in a > common way. > > e.g. for: > > > +static inline bool is_atomic(const volatile void *ptr) > > +{ > > + if (in_task()) { > > + if (unlikely(current->kcsan_atomic_next > 0)) { > > + --current->kcsan_atomic_next; > > + return true; > > + } > > + if (unlikely(current->kcsan_atomic_region > 0 || > > + current->kcsan_atomic_region_flat)) > > + return true; > > + } else { /* interrupt */ > > + if (unlikely(this_cpu_read(this_state.atomic_next) > 0)) { > > + this_cpu_dec(this_state.atomic_next); > > + return true; > > + } > > + if (unlikely(this_cpu_read(this_state.atomic_region) > 0 || > > + this_cpu_read(this_state.atomic_region_flat))) > > + return true; > > + } > > + > > + return kcsan_is_atomic(ptr); > > +} > > ... you could have something like: > > | struct kcsan_ctx *kcsan_get_ctx(void) > | { > | return in_task() ? ¤t->kcsan_ctx : this_cpu_ptr(kcsan_cpu_ctx); > | } > | > | static inline bool is_atomic(const volatile void *ptr) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | if (unlikely(ctx->atomic_next > 0) { > | --ctx->atomic_next; > | return true; > | } > | if (unlikely(ctx->atomic_region > 0 || ctx->atomic_region_flat)) > | return true; > | > | return kcsan_is_atomic(ptr); > | } > > ... avoiding duplicating the checks for task/irq contexts. > > It's not clear to me how either that or the original code works if a > softirq is interrupted by a hardirq. IIUC most of the fields should > remain stable over that window, since the hardirq should balance most > changes it makes before returning, but I don't think that's true for > atomic_next. Can't that be corrupted from the PoV of the softirq > handler? As you say, these fields should balance. So far I have not observed any issues. For atomic_next I'm not concerned as it is an approximation either way (see seqlock patch), and it's fine if there is a small error. > [...] > > > +void kcsan_begin_atomic(bool nest) > > +{ > > + if (nest) { > > + if (in_task()) > > + ++current->kcsan_atomic_region; > > + else > > + this_cpu_inc(this_state.atomic_region); > > + } else { > > + if (in_task()) > > + current->kcsan_atomic_region_flat = true; > > + else > > + this_cpu_write(this_state.atomic_region_flat, true); > > + } > > +} > > Assuming my suggestion above wasn't bogus, this can be: > > | void kcsan_begin_atomic(boot nest) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | if (nest) > | ctx->atomic_region++; > | else > | ctx->atomic_region_flat = true; > | } > > > +void kcsan_end_atomic(bool nest) > > +{ > > + if (nest) { > > + int prev = > > + in_task() ? > > + current->kcsan_atomic_region-- : > > + (this_cpu_dec_return(this_state.atomic_region) + > > + 1); > > + if (prev == 0) { > > + kcsan_begin_atomic(true); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > + kcsan_enable_current(); > > + } > > + } else { > > + if (in_task()) > > + current->kcsan_atomic_region_flat = false; > > + else > > + this_cpu_write(this_state.atomic_region_flat, false); > > + } > > +} > > ... similarly: > > | void kcsan_end_atomic(bool nest) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | > | if (nest) > | if (ctx->kcsan_atomic_region--) { > | kcsan_begin_atomic(true); /* restore to 0 */ > | kcsan_disable_current(); > | WARN(1, "mismatching %s"\ __func__); > | kcsan_enable_current(); > | } > | } else { > | ctx->atomic_region_flat = true; > | } > | } > > > +void kcsan_atomic_next(int n) > > +{ > > + if (in_task()) > > + current->kcsan_atomic_next = n; > > + else > > + this_cpu_write(this_state.atomic_next, n); > > +} > > ... and: > > | void kcsan_atomic_nextint n) > | { > | kcsan_get_ctx()->atomic_next = n; > | } Otherwise, yes, this makes much more sense and I will just introduce the struct and integrate the above suggestions for v2. Many thanks, -- Marco
WARNING: multiple messages have this Message-ID (diff)
From: Marco Elver <elver@google.com> To: Mark Rutland <mark.rutland@arm.com> Cc: LKMM Maintainers -- Akira Yokosawa <akiyks@gmail.com>, Alan Stern <stern@rowland.harvard.edu>, Alexander Potapenko <glider@google.com>, Andrea Parri <parri.andrea@gmail.com>, Andrey Konovalov <andreyknvl@google.com>, Andy Lutomirski <luto@kernel.org>, ard.biesheuvel@linaro.org, Arnd Bergmann <arnd@arndb.de>, Boqun Feng <boqun.feng@gmail.com>, Borislav Petkov <bp@alien8.de>, Daniel Axtens <dja@axtens.net>, Daniel Lustig <dlustig@nvidia.com>, dave.hansen@linux.intel.com, dhowells@redhat.com, Dmitry Vyukov <dvyukov@google.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, Jade Alglave <j.alglave@ucl.ac.uk>, Joel Fernandes <joel@joelfernandes.org>, Jonathan Corbet <corbet@lwn.net>, Josh Poimboeuf <jpoimboe@redhat.com> Subject: Re: [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure Date: Wed, 16 Oct 2019 17:53:45 +0200 [thread overview] Message-ID: <CANpmjNNctoVsUc+VbJ_RAMgLxcbvjq55gK1NdE0G0muMdv1+Ng@mail.gmail.com> (raw) In-Reply-To: <20191016151643.GC46264@lakrids.cambridge.arm.com> On Wed, 16 Oct 2019 at 17:16, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Oct 16, 2019 at 10:39:52AM +0200, Marco Elver wrote: > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 2c2e56bd8913..34a1d9310304 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -1171,6 +1171,13 @@ struct task_struct { > > #ifdef CONFIG_KASAN > > unsigned int kasan_depth; > > #endif > > +#ifdef CONFIG_KCSAN > > + /* See comments at kernel/kcsan/core.c: struct cpu_state. */ > > + int kcsan_disable; > > + int kcsan_atomic_next; > > + int kcsan_atomic_region; > > + bool kcsan_atomic_region_flat; > > +#endif > > Should these be unsigned? I prefer to keep them int, as they can become negative (rather than underflow with unsigned), if we e.g. have unbalanced kcsan_enable_current etc. Since we do not need the full unsigned range (these values should stay relatively small), int is more than enough. > > +/* > > + * Per-CPU state that should be used instead of 'current' if we are not in a > > + * task. > > + */ > > +struct cpu_state { > > + int disable; /* disable counter */ > > + int atomic_next; /* number of following atomic ops */ > > + > > + /* > > + * We use separate variables to store if we are in a nestable or flat > > + * atomic region. This helps make sure that an atomic region with > > + * nesting support is not suddenly aborted when a flat region is > > + * contained within. Effectively this allows supporting nesting flat > > + * atomic regions within an outer nestable atomic region. Support for > > + * this is required as there are cases where a seqlock reader critical > > + * section (flat atomic region) is contained within a seqlock writer > > + * critical section (nestable atomic region), and the "mismatching > > + * kcsan_end_atomic()" warning would trigger otherwise. > > + */ > > + int atomic_region; > > + bool atomic_region_flat; > > +}; > > +static DEFINE_PER_CPU(struct cpu_state, this_state) = { > > + .disable = 0, > > + .atomic_next = 0, > > + .atomic_region = 0, > > + .atomic_region_flat = 0, > > +}; > > These are the same as in task_struct, so I think it probably makes sense > to have a common structure for these, e.g. > > | struct kcsan_ctx { > | int disable; > | int atomic_next; > | int atomic_region; > | bool atomic_region_flat; > | }; > > ... which you then place within task_struct, e.g. > > | #ifdef CONFIG_KCSAN > | struct kcsan_ctx kcsan_ctx; > | #endif > > ... and here, e.g. > > | static DEFINE_PER_CPU(struct kcsan_ctx, kcsan_cpu_ctx); > > That would simplify a number of cases below where you have to choose one > or the other, as you can choose the pointer, then handle the rest in a > common way. > > e.g. for: > > > +static inline bool is_atomic(const volatile void *ptr) > > +{ > > + if (in_task()) { > > + if (unlikely(current->kcsan_atomic_next > 0)) { > > + --current->kcsan_atomic_next; > > + return true; > > + } > > + if (unlikely(current->kcsan_atomic_region > 0 || > > + current->kcsan_atomic_region_flat)) > > + return true; > > + } else { /* interrupt */ > > + if (unlikely(this_cpu_read(this_state.atomic_next) > 0)) { > > + this_cpu_dec(this_state.atomic_next); > > + return true; > > + } > > + if (unlikely(this_cpu_read(this_state.atomic_region) > 0 || > > + this_cpu_read(this_state.atomic_region_flat))) > > + return true; > > + } > > + > > + return kcsan_is_atomic(ptr); > > +} > > ... you could have something like: > > | struct kcsan_ctx *kcsan_get_ctx(void) > | { > | return in_task() ? ¤t->kcsan_ctx : this_cpu_ptr(kcsan_cpu_ctx); > | } > | > | static inline bool is_atomic(const volatile void *ptr) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | if (unlikely(ctx->atomic_next > 0) { > | --ctx->atomic_next; > | return true; > | } > | if (unlikely(ctx->atomic_region > 0 || ctx->atomic_region_flat)) > | return true; > | > | return kcsan_is_atomic(ptr); > | } > > ... avoiding duplicating the checks for task/irq contexts. > > It's not clear to me how either that or the original code works if a > softirq is interrupted by a hardirq. IIUC most of the fields should > remain stable over that window, since the hardirq should balance most > changes it makes before returning, but I don't think that's true for > atomic_next. Can't that be corrupted from the PoV of the softirq > handler? As you say, these fields should balance. So far I have not observed any issues. For atomic_next I'm not concerned as it is an approximation either way (see seqlock patch), and it's fine if there is a small error. > [...] > > > +void kcsan_begin_atomic(bool nest) > > +{ > > + if (nest) { > > + if (in_task()) > > + ++current->kcsan_atomic_region; > > + else > > + this_cpu_inc(this_state.atomic_region); > > + } else { > > + if (in_task()) > > + current->kcsan_atomic_region_flat = true; > > + else > > + this_cpu_write(this_state.atomic_region_flat, true); > > + } > > +} > > Assuming my suggestion above wasn't bogus, this can be: > > | void kcsan_begin_atomic(boot nest) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | if (nest) > | ctx->atomic_region++; > | else > | ctx->atomic_region_flat = true; > | } > > > +void kcsan_end_atomic(bool nest) > > +{ > > + if (nest) { > > + int prev = > > + in_task() ? > > + current->kcsan_atomic_region-- : > > + (this_cpu_dec_return(this_state.atomic_region) + > > + 1); > > + if (prev == 0) { > > + kcsan_begin_atomic(true); /* restore to 0 */ > > + kcsan_disable_current(); > > + WARN(1, "mismatching %s", __func__); > > + kcsan_enable_current(); > > + } > > + } else { > > + if (in_task()) > > + current->kcsan_atomic_region_flat = false; > > + else > > + this_cpu_write(this_state.atomic_region_flat, false); > > + } > > +} > > ... similarly: > > | void kcsan_end_atomic(bool nest) > | { > | struct kcsan_ctx *ctx = kcsan_get_ctx(); > | > | if (nest) > | if (ctx->kcsan_atomic_region--) { > | kcsan_begin_atomic(true); /* restore to 0 */ > | kcsan_disable_current(); > | WARN(1, "mismatching %s"\ __func__); > | kcsan_enable_current(); > | } > | } else { > | ctx->atomic_region_flat = true; > | } > | } > > > +void kcsan_atomic_next(int n) > > +{ > > + if (in_task()) > > + current->kcsan_atomic_next = n; > > + else > > + this_cpu_write(this_state.atomic_next, n); > > +} > > ... and: > > | void kcsan_atomic_nextint n) > | { > | kcsan_get_ctx()->atomic_next = n; > | } Otherwise, yes, this makes much more sense and I will just introduce the struct and integrate the above suggestions for v2. Many thanks, -- Marco
next prev parent reply other threads:[~2019-10-16 15:54 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-16 8:39 [PATCH 0/8] Add Kernel Concurrency Sanitizer (KCSAN) Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 9:42 ` Boqun Feng 2019-10-16 10:06 ` Marco Elver 2019-10-16 10:06 ` Marco Elver 2019-10-16 10:06 ` Marco Elver 2019-10-17 0:25 ` Boqun Feng 2019-10-17 0:25 ` Boqun Feng 2019-10-17 0:25 ` Boqun Feng 2019-10-16 11:49 ` Andrey Konovalov 2019-10-16 11:49 ` Andrey Konovalov 2019-10-16 11:49 ` Andrey Konovalov 2019-10-16 13:52 ` Marco Elver 2019-10-16 13:52 ` Marco Elver 2019-10-16 13:52 ` Marco Elver 2019-10-16 15:16 ` Mark Rutland 2019-10-16 15:53 ` Marco Elver [this message] 2019-10-16 15:53 ` Marco Elver 2019-10-16 15:53 ` Marco Elver 2019-10-16 18:43 ` Peter Zijlstra 2019-10-16 19:34 ` Marco Elver 2019-10-16 19:34 ` Marco Elver 2019-10-16 19:34 ` Marco Elver 2019-10-17 7:47 ` Peter Zijlstra 2019-10-17 7:47 ` Peter Zijlstra 2019-10-17 7:49 ` Marco Elver 2019-10-17 7:49 ` Marco Elver 2019-10-17 7:49 ` Marco Elver 2019-10-16 8:39 ` [PATCH 2/8] objtool, kcsan: Add KCSAN runtime functions to whitelist Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 3/8] build, kcsan: Add KCSAN build exceptions Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 4/8] seqlock, kcsan: Add annotations for KCSAN Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 5/8] seqlock: Require WRITE_ONCE surrounding raw_seqcount_barrier Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 6/8] asm-generic, kcsan: Add KCSAN instrumentation for bitops Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 8:39 ` [PATCH 7/8] locking/atomics, kcsan: Add KCSAN instrumentation Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 11:18 ` Mark Rutland 2019-10-16 11:47 ` Marco Elver 2019-10-16 11:47 ` Marco Elver 2019-10-16 11:47 ` Marco Elver 2019-10-16 8:39 ` [PATCH 8/8] x86, kcsan: Enable KCSAN for x86 Marco Elver 2019-10-16 8:39 ` Marco Elver 2019-10-16 16:14 ` Dave Hansen 2019-10-16 17:04 ` Marco Elver 2019-10-16 17:04 ` Marco Elver 2019-10-16 17:04 ` Marco Elver 2019-10-17 14:15 ` [PATCH 0/8] Add Kernel Concurrency Sanitizer (KCSAN) Marco Elver 2019-10-17 14:15 ` Marco Elver 2019-10-17 14:15 ` Marco Elver
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=CANpmjNNctoVsUc+VbJ_RAMgLxcbvjq55gK1NdE0G0muMdv1+Ng@mail.gmail.com \ --to=elver@google.com \ --cc=akiyks@gmail.com \ --cc=andreyknvl@google.com \ --cc=ard.biesheuvel@linaro.org \ --cc=arnd@arndb.de \ --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@linux.ibm.com \ --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.