Hi Mark,
Thanks for you comments; see inline comments below.
On Tue, 22 Oct 2019 at 16:11, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> Hi Marco,
>
> On Thu, Oct 17, 2019 at 04:12:58PM +0200, Marco Elver wrote:
> > Kernel Concurrency Sanitizer (KCSAN) is a dynamic data-race detector for
> > kernel space. KCSAN is a sampling watchpoint-based data-race detector.
> > See the included Documentation/dev-tools/kcsan.rst for more details.
> >
> > This patch adds basic infrastructure, but does not yet enable KCSAN for
> > any architecture.
> >
> > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > ---
> > v2:
> > * Elaborate comment about instrumentation calls emitted by compilers.
> > * Replace kcsan_check_access(.., {true, false}) with
> > kcsan_check_{read,write} for improved readability.
> > * Change bug title of race of unknown origin to just say "data-race in".
> > * Refine "Key Properties" in kcsan.rst, and mention observed slow-down.
> > * Add comment about safety of find_watchpoint without user_access_save.
> > * Remove unnecessary preempt_disable/enable and elaborate on comment why
> > we want to disable interrupts and preemptions.
> > * Use common struct kcsan_ctx in task_struct and for per-CPU interrupt
> > contexts [Suggested by Mark Rutland].
>
> This is generally looking good to me.
>
> I have a few comments below. Those are mostly style and naming things to
> minimize surprise, though I also have a couple of queries (nested vs
> flat atomic regions and the number of watchpoints).
>
> [...]
>
> > diff --git a/include/linux/kcsan.h b/include/linux/kcsan.h
> > new file mode 100644
> > index 000000000000..fd5de2ba3a16
> > --- /dev/null
> > +++ b/include/linux/kcsan.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_KCSAN_H
> > +#define _LINUX_KCSAN_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/kcsan-checks.h>
> > +
> > +#ifdef CONFIG_KCSAN
> > +
> > +/*
> > + * Context for each thread of execution: for tasks, this is stored in
> > + * task_struct, and interrupts access internal per-CPU storage.
> > + */
> > +struct kcsan_ctx {
> > + int disable; /* disable counter */
>
> Can we call this disable_count? That would match the convention used for
> preempt_count, and make it clear this isn't a boolean.
Done for v3.
> > + int atomic_next; /* number of following atomic ops */
>
> I'm a little unclear on why we need this given the begin ... end
> helpers -- isn't knowing that we're in an atomic region sufficient?
Sadly no, this is all due to seqlock usage. See seqlock patch for explanation.
> > +
> > + /*
> > + * 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;
> > +};
>
> I think we need to introduce nestability and flatness first. How about:
Thanks, updated wording to read better hopefully.
> /*
> * Some atomic sequences are flat, and cannot contain another
> * atomic sequence. Other atomic sequences are nestable, and may
> * contain other flat and/or nestable sequences.
> *
> * For example, a seqlock writer critical section is nestable
> * and may contain a seqlock reader critical section, which is
> * flat.
> *
> * To support this we track the depth of nesting, and whether
> * the leaf level is flat.
> */
> int atomic_nest_count;
> bool in_flat_atomic;
>
> That said, I'm not entirely clear on the distinction. Why would nesting
> a reader within another reader not be legitimate?
It is legitimate, however, seqlock reader critical sections do not
always have a balance begin/end. I ran into trouble initially when
readers were still nestable, as e.g. read_seqcount_retry can be called
multiple times. See seqlock patch for more explanations.
> > +
> > +/**
> > + * kcsan_init - initialize KCSAN runtime
> > + */
> > +void kcsan_init(void);
> > +
> > +/**
> > + * kcsan_disable_current - disable KCSAN for the current context
> > + *
> > + * Supports nesting.
> > + */
> > +void kcsan_disable_current(void);
> > +
> > +/**
> > + * kcsan_enable_current - re-enable KCSAN for the current context
> > + *
> > + * Supports nesting.
> > + */
> > +void kcsan_enable_current(void);
> > +
> > +/**
> > + * kcsan_begin_atomic - use to denote an atomic region
> > + *
> > + * Accesses within the atomic region may appear to race with other accesses but
> > + * should be considered atomic.
> > + *
> > + * @nest true if regions may be nested, or false for flat region
> > + */
> > +void kcsan_begin_atomic(bool nest);
> > +
> > +/**
> > + * kcsan_end_atomic - end atomic region
> > + *
> > + * @nest must match argument to kcsan_begin_atomic().
> > + */
> > +void kcsan_end_atomic(bool nest);
> > +
>
> Similarly to the check_{read,write}() naming, could we get rid of the
> bool argument and split this into separate nestable and flat functions?
>
> That makes it easier to read in-context, e.g.
>
> kcsan_nestable_atomic_begin();
> ...
> kcsan_nestable_atomic_end();
>
> ... has a more obvious meaning than:
>
> kcsan_begin_atomic(true);
> ...
> kcsan_end_atomic(true);
>
> ... and putting the begin/end at the end of the name makes it easier to
> spot the matching pair.
Thanks, done for v3.
> [...]
>
> > +static inline bool is_enabled(void)
> > +{
> > + return READ_ONCE(kcsan_enabled) && get_ctx()->disable == 0;
> > +}
>
> Can we please make this kcsan_is_enabled(), to avoid confusion with
> IS_ENABLED()?
Done for v3.
> > +static inline unsigned int get_delay(void)
> > +{
> > + unsigned int max_delay = in_task() ? CONFIG_KCSAN_UDELAY_MAX_TASK :
> > + CONFIG_KCSAN_UDELAY_MAX_INTERRUPT;
> > + return IS_ENABLED(CONFIG_KCSAN_DELAY_RANDOMIZE) ?
> > + ((prandom_u32() % max_delay) + 1) :
> > + max_delay;
> > +}
> > +
> > +/* === Public interface ===================================================== */
> > +
> > +void __init kcsan_init(void)
> > +{
> > + BUG_ON(!in_task());
> > +
> > + kcsan_debugfs_init();
> > + kcsan_enable_current();
> > +#ifdef CONFIG_KCSAN_EARLY_ENABLE
> > + /*
> > + * We are in the init task, and no other tasks should be running.
> > + */
> > + WRITE_ONCE(kcsan_enabled, true);
> > +#endif
>
> Where possible, please use IS_ENABLED() rather than ifdeffery for
> portions of functions like this, e.g.
>
> /*
> * We are in the init task, and no other tasks should be running.
> */
> if (IS_ENABLED(CONFIG_KCSAN_EARLY_ENABLE))
> WRITE_ONCE(kcsan_enabled, true);
>
> That makes code a bit easier to read, and ensures that the code always
> gets build coverage, so it's less likely that code changes will
> introduce a build failure when the option is enabled.
Thanks, done for v3.
> [...]
>
> > +#ifdef CONFIG_KCSAN_DEBUG
> > + kcsan_disable_current();
> > + pr_err("KCSAN: watching %s, size: %zu, addr: %px [slot: %d, encoded: %lx]\n",
> > + is_write ? "write" : "read", size, ptr,
> > + watchpoint_slot((unsigned long)ptr),
> > + encode_watchpoint((unsigned long)ptr, size, is_write));
> > + kcsan_enable_current();
> > +#endif
>
> This can use IS_ENABLED(), e.g.
>
> if (IS_ENABLED(CONFIG_KCSAN_DEBUG)) {
> kcsan_disable_current();
> pr_err("KCSAN: watching %s, size: %zu, addr: %px [slot: %d, encoded: %lx]\n",
> is_write ? "write" : "read", size, ptr,
> watchpoint_slot((unsigned long)ptr),
> encode_watchpoint((unsigned long)ptr, size, is_write));
> kcsan_enable_current();
> }
>
> [...]
> > +#ifdef CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
> > + kcsan_report(ptr, size, is_write, smp_processor_id(),
> > + kcsan_report_race_unknown_origin);
> > +#endif
>
> This can also use IS_ENABLED().
Done for v3.
> [...]
>
> > diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h
> > new file mode 100644
> > index 000000000000..429479b3041d
> > --- /dev/null
> > +++ b/kernel/kcsan/kcsan.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _MM_KCSAN_KCSAN_H
> > +#define _MM_KCSAN_KCSAN_H
> > +
> > +#include <linux/kcsan.h>
> > +
> > +/*
> > + * Total number of watchpoints. An address range maps into a specific slot as
> > + * specified in `encoding.h`. Although larger number of watchpoints may not even
> > + * be usable due to limited thread count, a larger value will improve
> > + * performance due to reducing cache-line contention.
> > + */
> > +#define KCSAN_NUM_WATCHPOINTS 64
>
> Is there any documentation as to how 64 was chosen? It's fine if it's
> arbitrary, but it would be good to know either way.
It was arbitrary in the sense that I chose the largest value that I
think is an acceptable overhead in terms of storage, i.e. on 64-bit
watchpoints consume 512 bytes. It should always be large enough so
that "no_capacity" counter does not increase frequently.
> I wonder if this is something that might need to scale with NR_CPUS (or
> nr_cpus).
I think this is hard to say. I've decided to make it configurable in
v3, with a BUILD_BUG_ON to ensure its value is within expected bounds.
> > +enum kcsan_counter_id {
> > + /*
> > + * Number of watchpoints currently in use.
> > + */
> > + kcsan_counter_used_watchpoints,
>
> Nit: typically enum values are capitalized (as coding-style.rst says).
> That helps to make it clear each value is a constant rather than a
> variable. Likewise for the other enums here.
Done for v3.
Thanks,
-- Marco
> Thanks,
> Mark.
