On Wed, 16 Oct 2019 at 17:16, Mark Rutland <mark.rutland@xxxxxxx> 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
