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=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable 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 F3B16CA9EBC for ; Thu, 24 Oct 2019 14:17:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id AA48321906 for ; Thu, 24 Oct 2019 14:17:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="rjhd/M55" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AA48321906 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 2F70A6B0005; Thu, 24 Oct 2019 10:17:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 27FF16B0007; Thu, 24 Oct 2019 10:17:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 147026B0008; Thu, 24 Oct 2019 10:17:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0066.hostedemail.com [216.40.44.66]) by kanga.kvack.org (Postfix) with ESMTP id E6F616B0005 for ; Thu, 24 Oct 2019 10:17:26 -0400 (EDT) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id 7C7F6181AC9BF for ; Thu, 24 Oct 2019 14:17:26 +0000 (UTC) X-FDA: 76078880892.15.ocean33_5dff3308ce256 X-HE-Tag: ocean33_5dff3308ce256 X-Filterd-Recvd-Size: 11028 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf21.hostedemail.com (Postfix) with ESMTP for ; Thu, 24 Oct 2019 14:17:25 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id 89so20750350oth.13 for ; Thu, 24 Oct 2019 07:17:25 -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=S8i+kPsaZWRIBXyQ2g5/YSOuQEjiriODMzIUUPGICqQ=; b=rjhd/M55ZS2DHpPVEyvmw+51VqoZKT+aKpBMcndrlavHNG5q2vOXiJVQS+s8EXv9Dd iCsoOVhsJ+G7gYkpFkn14zYdOpz0vSBpUzDsDu0PBCQ6NzT8xUq5cmrA4xocXCdFwB2z OY76EBmbTCadlVIzDU0S3rEb6LzKAsP2pySsCW8pNgaKuG8yg1KYKhaeOwpjz6pjIre3 ZEktUEMMOiAjH3feXIg6+YO0mzPvWpCriPh+vEPfhR6gnJPhxdJyw0QnglSg76VNRtTS 8IVxVkGqkZRoWCHuKSqjF3oWPtlHkYAoqyjWFNbEZcdU6FuP7HsXigSzaREmQGaHV4Ln 7pkA== 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=S8i+kPsaZWRIBXyQ2g5/YSOuQEjiriODMzIUUPGICqQ=; b=eVLefRNFEbMXE89zEyfgS3zkXhZlsYoOcbLlROcHhsXDqZ+6d4MdZZ86LbcvECYwZD n94zsarZqmzg2rYOg1OVdy3T7Z4BsCGe5Uvi8kyilX+Qwdc9TjNhm0Xv1vJqiRT3PXVr j/sMsXR8yU28dpk+jkpILg8STF+wfepGc0gBNbrnFTjAHK/fxtzoUTarZ8+bLI7BRsse R8M6ClLdFShDkwQCnMnU+oNE5tPcwu4QmLKuHBerVlU7/FvPXrpoP/TcuxbkhnIopZms TUUznjXQSxu1fHB0Qp0Ez7p4Dz3j6QldxJhO5K5Ob51cFWHF6A7abSPi2VYGNlNbbNGB cyzA== X-Gm-Message-State: APjAAAUz6uX77HMnkIVYyvkKSDdyjIhxs1IcH2exhhVgdbzyxHbAk3xH bdAIutwI+0/gSwaHpxuQtdYOxhY/T3iXVrOC4bnp8Q== X-Google-Smtp-Source: APXvYqzNSa3dJF91Ue/oxJ5CO5LETslsoJo48bQPunzQ1TmiLszytnOsFp8RpLz63hx9k13on0dq1OrbCD1kyNZI6S8= X-Received: by 2002:a9d:5f0f:: with SMTP id f15mr11239283oti.251.1571926644575; Thu, 24 Oct 2019 07:17:24 -0700 (PDT) MIME-Version: 1.0 References: <20191017141305.146193-1-elver@google.com> <20191017141305.146193-5-elver@google.com> <20191024122801.GD4300@lakrids.cambridge.arm.com> In-Reply-To: <20191024122801.GD4300@lakrids.cambridge.arm.com> From: Marco Elver Date: Thu, 24 Oct 2019 16:17:11 +0200 Message-ID: Subject: Re: [PATCH v2 4/8] seqlock, kcsan: Add annotations for KCSAN To: Mark Rutland 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 , David Howells , 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 mailing list , 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 Thu, 24 Oct 2019 at 14:28, Mark Rutland wrote: > > On Thu, Oct 17, 2019 at 04:13:01PM +0200, Marco Elver wrote: > > Since seqlocks in the Linux kernel do not require the use of marked > > atomic accesses in critical sections, we teach KCSAN to assume such > > accesses are atomic. KCSAN currently also pretends that writes to > > `sequence` are atomic, although currently plain writes are used (their > > corresponding reads are READ_ONCE). > > > > Further, to avoid false positives in the absence of clear ending of a > > seqlock reader critical section (only when using the raw interface), > > KCSAN assumes a fixed number of accesses after start of a seqlock > > critical section are atomic. > > Do we have many examples where there's not a clear end to a seqlock > sequence? Or are there just a handful? > > If there aren't that many, I wonder if we can make it mandatory to have > an explicit end, or to add some helper for those patterns so that we can > reliably hook them. In an ideal world, all usage of seqlocks would be via seqlock_t, which follows a somewhat saner usage, where we already do normal begin/end markings -- with subtle exception to readers needing to be flat atomic regions, e.g. because usage like this: - fs/namespace.c:__legitimize_mnt - unbalanced read_seqretry - fs/dcache.c:d_walk - unbalanced need_seqretry But anything directly accessing seqcount_t seems to be unpredictable. Filtering for usage of read_seqcount_retry not following 'do { .. } while (read_seqcount_retry(..));' (although even the ones in while loops aren't necessarily predictable): $ git grep 'read_seqcount_retry' | grep -Ev 'seqlock.h|Doc|\* ' | grep -v 'while (' => about 1/3 of the total read_seqcount_retry usage. Just looking at fs/namei.c, I would conclude that it'd be a pretty daunting task to prescribe and migrate to an interface that forces clear begin/end. Which is why I concluded that for now, it is probably better to make KCSAN play well with the existing code. Thanks, -- Marco > Thanks, > Mark. > > > > > Signed-off-by: Marco Elver > > --- > > include/linux/seqlock.h | 44 +++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 40 insertions(+), 4 deletions(-) > > > > diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h > > index bcf4cf26b8c8..1e425831a7ed 100644 > > --- a/include/linux/seqlock.h > > +++ b/include/linux/seqlock.h > > @@ -37,8 +37,24 @@ > > #include > > #include > > #include > > +#include > > #include > > > > +/* > > + * The seqlock interface does not prescribe a precise sequence of read > > + * begin/retry/end. For readers, typically there is a call to > > + * read_seqcount_begin() and read_seqcount_retry(), however, there are more > > + * esoteric cases which do not follow this pattern. > > + * > > + * As a consequence, we take the following best-effort approach for *raw* usage > > + * of seqlocks under KCSAN: upon beginning a seq-reader critical section, > > + * pessimistically mark then next KCSAN_SEQLOCK_REGION_MAX memory accesses as > > + * atomics; if there is a matching read_seqcount_retry() call, no following > > + * memory operations are considered atomic. Non-raw usage of seqlocks is not > > + * affected. > > + */ > > +#define KCSAN_SEQLOCK_REGION_MAX 1000 > > + > > /* > > * Version using sequence counter only. > > * This can be used when code has its own mutex protecting the > > @@ -115,6 +131,7 @@ static inline unsigned __read_seqcount_begin(const seqcount_t *s) > > cpu_relax(); > > goto repeat; > > } > > + kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX); > > return ret; > > } > > > > @@ -131,6 +148,7 @@ static inline unsigned raw_read_seqcount(const seqcount_t *s) > > { > > unsigned ret = READ_ONCE(s->sequence); > > smp_rmb(); > > + kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX); > > return ret; > > } > > > > @@ -183,6 +201,7 @@ static inline unsigned raw_seqcount_begin(const seqcount_t *s) > > { > > unsigned ret = READ_ONCE(s->sequence); > > smp_rmb(); > > + kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX); > > return ret & ~1; > > } > > > > @@ -202,7 +221,8 @@ static inline unsigned raw_seqcount_begin(const seqcount_t *s) > > */ > > static inline int __read_seqcount_retry(const seqcount_t *s, unsigned start) > > { > > - return unlikely(s->sequence != start); > > + kcsan_atomic_next(0); > > + return unlikely(READ_ONCE(s->sequence) != start); > > } > > > > /** > > @@ -225,6 +245,7 @@ static inline int read_seqcount_retry(const seqcount_t *s, unsigned start) > > > > static inline void raw_write_seqcount_begin(seqcount_t *s) > > { > > + kcsan_begin_atomic(true); > > s->sequence++; > > smp_wmb(); > > } > > @@ -233,6 +254,7 @@ static inline void raw_write_seqcount_end(seqcount_t *s) > > { > > smp_wmb(); > > s->sequence++; > > + kcsan_end_atomic(true); > > } > > > > /** > > @@ -262,18 +284,20 @@ static inline void raw_write_seqcount_end(seqcount_t *s) > > * > > * void write(void) > > * { > > - * Y = true; > > + * WRITE_ONCE(Y, true); > > * > > * raw_write_seqcount_barrier(seq); > > * > > - * X = false; > > + * WRITE_ONCE(X, false); > > * } > > */ > > static inline void raw_write_seqcount_barrier(seqcount_t *s) > > { > > + kcsan_begin_atomic(true); > > s->sequence++; > > smp_wmb(); > > s->sequence++; > > + kcsan_end_atomic(true); > > } > > > > static inline int raw_read_seqcount_latch(seqcount_t *s) > > @@ -398,7 +422,9 @@ static inline void write_seqcount_end(seqcount_t *s) > > static inline void write_seqcount_invalidate(seqcount_t *s) > > { > > smp_wmb(); > > + kcsan_begin_atomic(true); > > s->sequence+=2; > > + kcsan_end_atomic(true); > > } > > > > typedef struct { > > @@ -430,11 +456,21 @@ typedef struct { > > */ > > static inline unsigned read_seqbegin(const seqlock_t *sl) > > { > > - return read_seqcount_begin(&sl->seqcount); > > + unsigned ret = read_seqcount_begin(&sl->seqcount); > > + > > + kcsan_atomic_next(0); /* non-raw usage, assume closing read_seqretry */ > > + kcsan_begin_atomic(false); > > + return ret; > > } > > > > static inline unsigned read_seqretry(const seqlock_t *sl, unsigned start) > > { > > + /* > > + * Assume not nested: read_seqretry may be called multiple times when > > + * completing read critical section. > > + */ > > + kcsan_end_atomic(false); > > + > > return read_seqcount_retry(&sl->seqcount, start); > > } > > > > -- > > 2.23.0.866.gb869b98d4c-goog > >