Message-ID: <CANpmjNP4b9Eo3ZKE6maBs4ANS7K7sLiVB2CbebQnCH09TB+hZQ@mail.gmail.com>
Date:   Wed, 16 Oct 2019 21:34:05 +0200
From:   Marco Elver <elver@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     LKMM Maintainers -- Akira Yokosawa <akiyks@...il.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Alexander Potapenko <glider@...gle.com>,
        Andrea Parri <parri.andrea@...il.com>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Boqun Feng <boqun.feng@...il.com>,
        Borislav Petkov <bp@...en8.de>, Daniel Axtens <dja@...ens.net>,
        Daniel Lustig <dlustig@...dia.com>,
        dave.hansen@...ux.intel.com, David Howells <dhowells@...hat.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Jade Alglave <j.alglave@....ac.uk>,
        Joel Fernandes <joel@...lfernandes.org>,
        Jonathan Corbet <corbet@....net>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Luc Maranget <luc.maranget@...ia.fr>,
        Mark Rutland <mark.rutland@....com>,
        Nicholas Piggin <npiggin@...il.com>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        linux-efi@...r.kernel.org,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        "the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure

On Wed, 16 Oct 2019 at 20:44, Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Wed, Oct 16, 2019 at 10:39:52AM +0200, Marco Elver wrote:
>
> > +bool __kcsan_check_watchpoint(const volatile void *ptr, size_t size,
> > +                           bool is_write)
> > +{
> > +     atomic_long_t *watchpoint;
> > +     long encoded_watchpoint;
> > +     unsigned long flags;
> > +     enum kcsan_report_type report_type;
> > +
> > +     if (unlikely(!is_enabled()))
> > +             return false;
> > +
> > +     watchpoint = find_watchpoint((unsigned long)ptr, size, !is_write,
> > +                                  &encoded_watchpoint);
> > +     if (watchpoint == NULL)
> > +             return true;
> > +
> > +     flags = user_access_save();
>
> Could use a comment on why find_watchpoint() is save to call without
> user_access_save() on.

Thanks, will add a comment for v2.

> > +     if (!try_consume_watchpoint(watchpoint, encoded_watchpoint)) {
> > +             /*
> > +              * The other thread may not print any diagnostics, as it has
> > +              * already removed the watchpoint, or another thread consumed
> > +              * the watchpoint before this thread.
> > +              */
> > +             kcsan_counter_inc(kcsan_counter_report_races);
> > +             report_type = kcsan_report_race_check_race;
> > +     } else {
> > +             report_type = kcsan_report_race_check;
> > +     }
> > +
> > +     /* Encountered a data-race. */
> > +     kcsan_counter_inc(kcsan_counter_data_races);
> > +     kcsan_report(ptr, size, is_write, raw_smp_processor_id(), report_type);
> > +
> > +     user_access_restore(flags);
> > +     return false;
> > +}
> > +EXPORT_SYMBOL(__kcsan_check_watchpoint);
> > +
> > +void __kcsan_setup_watchpoint(const volatile void *ptr, size_t size,
> > +                           bool is_write)
> > +{
> > +     atomic_long_t *watchpoint;
> > +     union {
> > +             u8 _1;
> > +             u16 _2;
> > +             u32 _4;
> > +             u64 _8;
> > +     } expect_value;
> > +     bool is_expected = true;
> > +     unsigned long ua_flags = user_access_save();
> > +     unsigned long irq_flags;
> > +
> > +     if (!should_watch(ptr))
> > +             goto out;
> > +
> > +     if (!check_encodable((unsigned long)ptr, size)) {
> > +             kcsan_counter_inc(kcsan_counter_unencodable_accesses);
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Disable interrupts & preemptions, to ignore races due to accesses in
> > +      * threads running on the same CPU.
> > +      */
> > +     local_irq_save(irq_flags);
> > +     preempt_disable();
>
> Is there a point to that preempt_disable() here?

We want to avoid being preempted while the watchpoint is set up;
otherwise, we would report data-races for CPU-local data, which is
incorrect. An alternative would be adding the source CPU to the
watchpoint, and checking that the CPU != this_cpu. There are several
problems with that alternative:
1. We do not want to steal more bits from the watchpoint encoding for
things other than read/write, size, and address, as not only does it
affect accuracy, it would also increase performance overhead in the
fast-path.
2. As a consequence, if we get a preemption and run a task on the same
CPU, and there *is* a genuine data-race, we would *not* report it; and
since this is the common case (and not accesses to CPU-local data), it
makes more sense (from a data-race detection PoV) to simply disable
preemptions and ensure that all tasks are run on other CPUs as well as
avoid the problem of point (1).

I can add a comment to that effect here for v2.

Thanks,
-- Marco

Powered by blists - more mailing lists