From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60D1CECE599 for ; Wed, 16 Oct 2019 19:34:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 329692064B for ; Wed, 16 Oct 2019 19:34:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="U7NFsQMF" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2391069AbfJPTeU (ORCPT ); Wed, 16 Oct 2019 15:34:20 -0400 Received: from mail-ot1-f68.google.com ([209.85.210.68]:46966 "EHLO mail-ot1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389320AbfJPTeT (ORCPT ); Wed, 16 Oct 2019 15:34:19 -0400 Received: by mail-ot1-f68.google.com with SMTP id 89so21186799oth.13 for ; Wed, 16 Oct 2019 12:34:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RXr4UV4kEHsq0yeWfqjzxiqcNiJQEejUfsbyhCIH4rQ=; b=U7NFsQMFohcPs28AZ7A+6GhhiEGUKgu+5pnJk2F9HrfSIYZCF5qXVKvKhW5L+RoOHB jqP0typxYPq6W5WuAp2s6t9gy8ue0IU9qaNy4j1gM5uq+Qeq1W1+L+X+MHRTfN2uCOcV uYAeJEYyAMziJ0JNWN0sw/aeCFN1uSv0dzQ7r0bIHjXvNLjLLlaipqpEUWVxI908shfo s0snQyTNsaYOQ06/yoSQNXPmDX4JhRWOBTdrA4MJAs0i/4AGDjE7OzTnwubKSQUdOlty scrRf9IM2XIfHvmJqZAaKmV46KAR/82a8OhMDL3dHf6x5PKKodh18mYX/2rhl3tYtP+s cfCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RXr4UV4kEHsq0yeWfqjzxiqcNiJQEejUfsbyhCIH4rQ=; b=q6RdBrZTv+yB9bxtiys48Cok+sokEIOaPXPOIBPnEeIhdlFHA/2O+9n/TRacBuh+Vs 128htdmRzumQHDRRgCdGmDjDSVPDEEdrSeL9yN+UQNt6lz6nJI1WHCaIL9KkYGOcHKpj hEblEDHg+yp5tf6yYabaGpi1qRBHeNfFKYWG5rfvD7RYu6iTCV8OxHaXR8rVvrtZQ+C8 gmFwqU+1yaTFPp9FHHwclJ7bcM78Z3UC0WSiSQYhsFuYjPt2RbCAm9MDMjRIHj2k07q3 DhtrUlVzbFHlKb9fsXpYwryj1wFZIHvRd2uRy4qFZOx1CaknrJd2lUGaBsO6QfPju91g yjBg== X-Gm-Message-State: APjAAAV4UmeGOm3NNqWbzPHrX+c+MpVVDulxREmpJxGLS0X4IxUShiGg xTH/bsC5HHca4Gh+YV3ANoNm0HoAHpTfpSssxWbImA== X-Google-Smtp-Source: APXvYqyqs6Tx0r7D3LhzsgcxEVO4XgI1nK8AQjvDBJBM9adnKIKx1Vz8ue7z1QVFQNbxwHJobnt3PfeMbB8stLkt8vM= X-Received: by 2002:a9d:6d89:: with SMTP id x9mr31120620otp.17.1571254457140; Wed, 16 Oct 2019 12:34:17 -0700 (PDT) MIME-Version: 1.0 References: <20191016083959.186860-1-elver@google.com> <20191016083959.186860-2-elver@google.com> <20191016184346.GT2328@hirez.programming.kicks-ass.net> In-Reply-To: <20191016184346.GT2328@hirez.programming.kicks-ass.net> From: Marco Elver Date: Wed, 16 Oct 2019 21:34:05 +0200 Message-ID: Subject: Re: [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure To: Peter Zijlstra Cc: LKMM Maintainers -- Akira Yokosawa , Alan Stern , Alexander Potapenko , Andrea Parri , Andrey Konovalov , Andy Lutomirski , Ard Biesheuvel , Arnd Bergmann , Boqun Feng , Borislav Petkov , Daniel Axtens , Daniel Lustig , dave.hansen@linux.intel.com, David Howells , Dmitry Vyukov , "H. Peter Anvin" , Ingo Molnar , Jade Alglave , Joel Fernandes , Jonathan Corbet , Josh Poimboeuf , Luc Maranget , Mark Rutland , Nicholas Piggin , "Paul E. McKenney" , Thomas Gleixner , Will Deacon , kasan-dev , linux-arch , "open list:DOCUMENTATION" , linux-efi@vger.kernel.org, Linux Kbuild mailing list , LKML , Linux Memory Management List , "the arch/x86 maintainers" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 16 Oct 2019 at 20:44, Peter Zijlstra 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