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=-11.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,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 1BB53FA372B for ; Wed, 16 Oct 2019 15:54:01 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C886C21835 for ; Wed, 16 Oct 2019 15:54:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="oO69GKlE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C886C21835 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 48CAE8E0005; Wed, 16 Oct 2019 11:54:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 43C318E0001; Wed, 16 Oct 2019 11:54:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2DC328E0005; Wed, 16 Oct 2019 11:54:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0155.hostedemail.com [216.40.44.155]) by kanga.kvack.org (Postfix) with ESMTP id 050738E0001 for ; Wed, 16 Oct 2019 11:53:59 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with SMTP id 926F51839A9D4 for ; Wed, 16 Oct 2019 15:53:59 +0000 (UTC) X-FDA: 76050093798.13.glass83_22c340cc8b20b X-HE-Tag: glass83_22c340cc8b20b X-Filterd-Recvd-Size: 11973 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Wed, 16 Oct 2019 15:53:58 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id z6so20594976otb.2 for ; Wed, 16 Oct 2019 08:53:58 -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=ES8PPxqISv6le2aU+Na1jJhECeSWLsXBsBUpCljFmNc=; b=oO69GKlEUEmZWfog28/tduGmrNxTECrWedf44ep3BiJWADibe3BtF/qSPOt+lHQnrd HMZ/O6vy/oZ1Mn29CFgrBvva1cxYf0fsP2Iv2I85mFP+PSSHxMWS5bWwvJtAkW4KeQ4X zHsZ4zn4Ud3sy3FtVlyWWHfups+bSfs5dcKPEQoYxyP86YUHE43Ssz5B42ovDjgprbuJ QoImASQ3r2C39DI1HTT9w7MtYoLRwjzHESnKd/q21EZ4qeAz9arZ/6uRGqW264QE2D9u M4kZpDAKBF3fk3pugbmFA+dNIL07VCSKkOnXhdXNc50ltBiqs8ufauu0rzHnctKYwyTf J+zg== 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=ES8PPxqISv6le2aU+Na1jJhECeSWLsXBsBUpCljFmNc=; b=Xw0tuA/YvE2G/w/Sjh0q3bAFhCcYo2SxrD5wcwhlGXqOFcZV6zTpPQXlS/VyArWHSx COJdkXdA7VvPcNHVJuqI1p9Exi2xNH6ghNioEqhUmquFe+pebett7JqZ7eukZJ62qewp kM0B4llwNBeSZAfWg5CnO88Ula170h96zkoB9nWdDZ5JB84iRqbHBvKEzwHCMRkXpu0W 6l8d7ZnXUE2fkB3jWkBY43mI6UAKSIiWbNey6ngCTSIwsehZvGiwcslF3lVAXandCsSn V2ztjRBE9jC7miW2ECk1/QrXZ/LzYI/eZf0CKf0MvqMbuNeuQM5U0326ZzC63Syx7nIs E59w== X-Gm-Message-State: APjAAAXRYLYwwUklQUk6DKJMoeUDJs+YtuUpyliaA11Ji+w7T3lSY3D1 gn+fhQ+PDImgtTVKpm9WuUVcApMye7CsjqQxnQhq3A== X-Google-Smtp-Source: APXvYqy/bEpTtLLHRnuZTAKQyMKjVxdlLHEt24tIzExwmKRAvwqnmZoQehyCbZB5w1Ds0iSmKpp3CZhKegjg3ge7PUQ= X-Received: by 2002:a9d:7590:: with SMTP id s16mr8514934otk.2.1571241237486; Wed, 16 Oct 2019 08:53:57 -0700 (PDT) MIME-Version: 1.0 References: <20191016083959.186860-1-elver@google.com> <20191016083959.186860-2-elver@google.com> <20191016151643.GC46264@lakrids.cambridge.arm.com> In-Reply-To: <20191016151643.GC46264@lakrids.cambridge.arm.com> From: Marco Elver Date: Wed, 16 Oct 2019 17:53:45 +0200 Message-ID: Subject: Re: [PATCH 1/8] kcsan: Add Kernel Concurrency Sanitizer infrastructure To: Mark Rutland Cc: LKMM Maintainers -- Akira Yokosawa , Alan Stern , Alexander Potapenko , Andrea Parri , Andrey Konovalov , Andy Lutomirski , ard.biesheuvel@linaro.org, Arnd Bergmann , Boqun Feng , Borislav Petkov , Daniel Axtens , Daniel Lustig , dave.hansen@linux.intel.com, dhowells@redhat.com, Dmitry Vyukov , "H. Peter Anvin" , Ingo Molnar , Jade Alglave , Joel Fernandes , Jonathan Corbet , Josh Poimboeuf , Luc Maranget , Nicholas Piggin , "Paul E. McKenney" , Peter Zijlstra , Thomas Gleixner , Will Deacon , kasan-dev , linux-arch , "open list:DOCUMENTATION" , linux-efi@vger.kernel.org, linux-kbuild@vger.kernel.org, LKML , Linux Memory Management List , "the arch/x86 maintainers" Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, 16 Oct 2019 at 17:16, Mark Rutland 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