From: "Paul E. McKenney" <paulmck@kernel.org>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: Marco Elver <elver@google.com>,
Alexander Potapenko <glider@google.com>,
Borislav Petkov <bp@alien8.de>,
Dmitry Vyukov <dvyukov@google.com>,
Ingo Molnar <mingo@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
Waiman Long <longman@redhat.com>, Will Deacon <will@kernel.org>,
kasan-dev@googlegroups.com, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kbuild@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
llvm@lists.linux.dev, x86@kernel.org
Subject: Re: [PATCH v3 08/25] kcsan: Show location access was reordered to
Date: Mon, 6 Dec 2021 09:38:59 -0800 [thread overview]
Message-ID: <20211206173859.GA641268@paulmck-ThinkPad-P17-Gen-1> (raw)
In-Reply-To: <Ya5FaU9e6XY8vHJR@boqun-archlinux>
On Tue, Dec 07, 2021 at 01:16:25AM +0800, Boqun Feng wrote:
> On Mon, Dec 06, 2021 at 05:04:20PM +0100, Marco Elver wrote:
> > On Mon, Dec 06, 2021 at 10:31PM +0800, Boqun Feng wrote:
> > [...]
> > > Thanks for the explanation, I was missing the swap here. However...
> > >
> > > > So in your above example you need to swap "reordered to" and the top
> > > > frame of the stack trace.
> > > >
> >
> > Apologies, I wasn't entirely precise ... what you say below is correct.
> >
> > > IIUC, the report for my above example will be:
> > >
> > > | write (reordered) to 0xaaaa of ...:
> > > | foo+0x... // address of the write to A
> > > | ...
> > > | |
> > > | +-> reordered to: foo+0x... // address of the callsite to bar() in foo()
> > >
> > > , right? Because in replace_stack_entry(), it's not the top frame where
> > > the race occurred that gets swapped, it's the frame which belongs to the
> > > same function as the original access that gets swapped. In other words,
> > > when KCSAN finds the problem, top entries of the calling stack are:
> > >
> > > [0] bar+0x.. // address of the write to B
> > > [1] foo+0x.. // address of the callsite to bar() in foo()
> > >
> > > after replace_stack_entry(), they changes to:
> > >
> > > [0] bar+0x.. // address of the write to B
> > > skip ->[1] foo+0x.. // address of the write to A
> > >
> > > , as a result the report won't mention bar() at all.
> >
> > Correct.
> >
> > > And I think a better report will be:
> > >
> > > | write (reordered) to 0xaaaa of ...:
> > > | foo+0x... // address of the write to A
> > > | ...
> > > | |
> > > | +-> reordered to: bar+0x... // address of the write to B in bar()
> > >
> > > because it tells users the exact place the accesses get reordered. That
> > > means maybe we want something as below? Not completely tested, but I
> > > play with scope checking a bit, seems it gives what I want. Thoughts?
> >
> > This is problematic because it makes it much harder to actually figure
> > out what's going on, given "reordered to" isn't a full stack trace. So
> > if you're deep in some call hierarchy, seeing a random "reordered to"
> > line is quite useless. What I want to see, at the very least, is the ip
> > to the same function where the original access happened.
> >
> > We could of course try and generate a full stack trace at "reordered
> > to", but this would entail
> >
> > a) allocating 2x unsigned long[64] on the stack (or moving to
> > static storage),
> > b) further increasing the report length,
> > c) an even larger number of possibly distinct reports for the
> > same issue; this makes deduplication even harder.
> >
> > The reason I couldn't justify all that is that when I looked through
> > several dozen "reordered to" reports, I never found anything other than
> > the ip in the function frame of the original access useful. That, and in
> > most cases the "reordered to" location was in the same function or in an
> > inlined function.
> >
> > The below patch would do what you'd want I think.
> >
> > My opinion is to err on the side of simplicity until there is evidence
> > we need it. Of course, if you have a compelling reason that we need it
> > from the beginning, happy to send it as a separate patch on top.
> >
> > What do you think?
> >
>
> Totally agreed. It's better to keep it simple until people report that
> they want to see more information to resolve the issues. And thanks for
> looking into the "double stack traces", that looks good to me too.
>
> For the original patch, feel free to add:
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Thank you both! I will add Boqun's Reviewed-by on the next rebase.
Thanx, Paul
> Regards,
> Boqun
>
> > Thanks,
> > -- Marco
> >
> > ------ >8 ------
> >
> > From: Marco Elver <elver@google.com>
> > Date: Mon, 6 Dec 2021 16:35:02 +0100
> > Subject: [PATCH] kcsan: Show full stack trace of reordered-to accesses
> >
> > Change reports involving reordered accesses to show the full stack trace
> > of "reordered to" accesses. For example:
> >
> > | ==================================================================
> > | BUG: KCSAN: data-race in test_kernel_wrong_memorder / test_kernel_wrong_memorder
> > |
> > | read-write to 0xffffffffc02d01e8 of 8 bytes by task 2481 on cpu 2:
> > | test_kernel_wrong_memorder+0x57/0x90
> > | access_thread+0xb7/0x100
> > | kthread+0x2ed/0x320
> > | ret_from_fork+0x22/0x30
> > |
> > | read-write (reordered) to 0xffffffffc02d01e8 of 8 bytes by task 2480 on cpu 0:
> > | test_kernel_wrong_memorder+0x57/0x90
> > | access_thread+0xb7/0x100
> > | kthread+0x2ed/0x320
> > | ret_from_fork+0x22/0x30
> > | |
> > | +-> reordered to: test_delay+0x31/0x110
> > | test_kernel_wrong_memorder+0x80/0x90
> > |
> > | Reported by Kernel Concurrency Sanitizer on:
> > | CPU: 0 PID: 2480 Comm: access_thread Not tainted 5.16.0-rc1+ #2
> > | Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
> > | ==================================================================
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> > kernel/kcsan/report.c | 33 +++++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c
> > index 67794404042a..a8317d5f5123 100644
> > --- a/kernel/kcsan/report.c
> > +++ b/kernel/kcsan/report.c
> > @@ -317,22 +317,29 @@ replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned lon
> > {
> > unsigned long symbolsize, offset;
> > unsigned long target_func;
> > - int skip;
> > + int skip, i;
> >
> > if (kallsyms_lookup_size_offset(ip, &symbolsize, &offset))
> > target_func = ip - offset;
> > else
> > goto fallback;
> >
> > - for (skip = 0; skip < num_entries; ++skip) {
> > + skip = get_stack_skipnr(stack_entries, num_entries);
> > + for (i = 0; skip < num_entries; ++skip, ++i) {
> > unsigned long func = stack_entries[skip];
> >
> > if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset))
> > goto fallback;
> > func -= offset;
> >
> > + replaced[i] = stack_entries[skip];
> > if (func == target_func) {
> > - *replaced = stack_entries[skip];
> > + /*
> > + * There must be at least 1 entry left in the original
> > + * @stack_entries, so we know that we will never occupy
> > + * more than @num_entries - 1 of @replaced.
> > + */
> > + replaced[i + 1] = 0;
> > stack_entries[skip] = ip;
> > return skip;
> > }
> > @@ -341,6 +348,7 @@ replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned lon
> > fallback:
> > /* Should not happen; the resulting stack trace is likely misleading. */
> > WARN_ONCE(1, "Cannot find frame for %pS in stack trace", (void *)ip);
> > + replaced[0] = 0;
> > return get_stack_skipnr(stack_entries, num_entries);
> > }
> >
> > @@ -365,11 +373,16 @@ static int sym_strcmp(void *addr1, void *addr2)
> > }
> >
> > static void
> > -print_stack_trace(unsigned long stack_entries[], int num_entries, unsigned long reordered_to)
> > +print_stack_trace(unsigned long stack_entries[], int num_entries, unsigned long *reordered_to)
> > {
> > stack_trace_print(stack_entries, num_entries, 0);
> > - if (reordered_to)
> > - pr_err(" |\n +-> reordered to: %pS\n", (void *)reordered_to);
> > + if (reordered_to[0]) {
> > + int i;
> > +
> > + pr_err(" |\n +-> reordered to: %pS\n", (void *)reordered_to[0]);
> > + for (i = 1; i < NUM_STACK_ENTRIES && reordered_to[i]; ++i)
> > + pr_err(" %pS\n", (void *)reordered_to[i]);
> > + }
> > }
> >
> > static void print_verbose_info(struct task_struct *task)
> > @@ -390,12 +403,12 @@ static void print_report(enum kcsan_value_change value_change,
> > struct other_info *other_info,
> > u64 old, u64 new, u64 mask)
> > {
> > - unsigned long reordered_to = 0;
> > + unsigned long reordered_to[NUM_STACK_ENTRIES] = { 0 };
> > unsigned long stack_entries[NUM_STACK_ENTRIES] = { 0 };
> > int num_stack_entries = stack_trace_save(stack_entries, NUM_STACK_ENTRIES, 1);
> > - int skipnr = sanitize_stack_entries(stack_entries, num_stack_entries, ai->ip, &reordered_to);
> > + int skipnr = sanitize_stack_entries(stack_entries, num_stack_entries, ai->ip, reordered_to);
> > unsigned long this_frame = stack_entries[skipnr];
> > - unsigned long other_reordered_to = 0;
> > + unsigned long other_reordered_to[NUM_STACK_ENTRIES] = { 0 };
> > unsigned long other_frame = 0;
> > int other_skipnr = 0; /* silence uninit warnings */
> >
> > @@ -408,7 +421,7 @@ static void print_report(enum kcsan_value_change value_change,
> > if (other_info) {
> > other_skipnr = sanitize_stack_entries(other_info->stack_entries,
> > other_info->num_stack_entries,
> > - other_info->ai.ip, &other_reordered_to);
> > + other_info->ai.ip, other_reordered_to);
> > other_frame = other_info->stack_entries[other_skipnr];
> >
> > /* @value_change is only known for the other thread */
> > --
> > 2.34.1.400.ga245620fadb-goog
> >
next prev parent reply other threads:[~2021-12-06 17:39 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-30 11:44 [PATCH v3 00/25] kcsan: Support detecting a subset of missing memory barriers Marco Elver
2021-11-30 11:44 ` [PATCH v3 01/25] kcsan: Refactor reading of instrumented memory Marco Elver
2021-11-30 11:44 ` [PATCH v3 02/25] kcsan: Remove redundant zero-initialization of globals Marco Elver
2021-11-30 11:44 ` [PATCH v3 03/25] kcsan: Avoid checking scoped accesses from nested contexts Marco Elver
2021-11-30 11:44 ` [PATCH v3 04/25] kcsan: Add core support for a subset of weak memory modeling Marco Elver
2021-12-03 8:56 ` Marco Elver
2021-12-03 16:50 ` Paul E. McKenney
2021-12-03 21:08 ` Paul E. McKenney
2021-12-03 23:42 ` Marco Elver
2021-12-03 23:42 ` Paul E. McKenney
2021-12-03 23:45 ` Marco Elver
2021-12-04 1:14 ` Paul E. McKenney
2021-11-30 11:44 ` [PATCH v3 05/25] kcsan: Add core memory barrier instrumentation functions Marco Elver
2021-11-30 11:44 ` [PATCH v3 06/25] kcsan, kbuild: Add option for barrier instrumentation only Marco Elver
2021-11-30 11:44 ` [PATCH v3 07/25] kcsan: Call scoped accesses reordered in reports Marco Elver
2021-11-30 11:44 ` [PATCH v3 08/25] kcsan: Show location access was reordered to Marco Elver
2021-12-06 5:03 ` Boqun Feng
2021-12-06 7:16 ` Marco Elver
2021-12-06 14:31 ` Boqun Feng
2021-12-06 16:04 ` Marco Elver
2021-12-06 17:16 ` Boqun Feng
2021-12-06 17:38 ` Paul E. McKenney [this message]
2021-11-30 11:44 ` [PATCH v3 09/25] kcsan: Document modeling of weak memory Marco Elver
2021-11-30 11:44 ` [PATCH v3 10/25] kcsan: test: Match reordered or normal accesses Marco Elver
2021-11-30 11:44 ` [PATCH v3 11/25] kcsan: test: Add test cases for memory barrier instrumentation Marco Elver
2021-11-30 11:44 ` [PATCH v3 12/25] kcsan: Ignore GCC 11+ warnings about TSan runtime support Marco Elver
2021-11-30 11:44 ` [PATCH v3 13/25] kcsan: selftest: Add test case to check memory barrier instrumentation Marco Elver
2021-11-30 11:44 ` [PATCH v3 14/25] locking/barriers, kcsan: Add instrumentation for barriers Marco Elver
2021-11-30 11:44 ` [PATCH v3 15/25] locking/barriers, kcsan: Support generic instrumentation Marco Elver
2021-11-30 11:44 ` [PATCH v3 16/25] locking/atomics, kcsan: Add instrumentation for barriers Marco Elver
2021-11-30 11:44 ` [PATCH v3 17/25] asm-generic/bitops, " Marco Elver
2021-11-30 11:44 ` [PATCH v3 18/25] x86/barriers, kcsan: Use generic instrumentation for non-smp barriers Marco Elver
2021-11-30 11:44 ` [PATCH v3 19/25] x86/qspinlock, kcsan: Instrument barrier of pv_queued_spin_unlock() Marco Elver
2021-11-30 11:44 ` [PATCH v3 20/25] mm, kcsan: Enable barrier instrumentation Marco Elver
2021-11-30 11:44 ` [PATCH v3 21/25] sched, kcsan: Enable memory " Marco Elver
2021-11-30 11:44 ` [PATCH v3 22/25] objtool, kcsan: Add memory barrier instrumentation to whitelist Marco Elver
2021-11-30 11:44 ` [PATCH v3 23/25] objtool, kcsan: Remove memory barrier instrumentation from noinstr Marco Elver
2021-11-30 11:44 ` [PATCH v3 24/25] compiler_attributes.h: Add __disable_sanitizer_instrumentation Marco Elver
2021-11-30 11:44 ` [PATCH v3 25/25] kcsan: Support WEAK_MEMORY with Clang where no objtool support exists Marco Elver
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211206173859.GA641268@paulmck-ThinkPad-P17-Gen-1 \
--to=paulmck@kernel.org \
--cc=boqun.feng@gmail.com \
--cc=bp@alien8.de \
--cc=dvyukov@google.com \
--cc=elver@google.com \
--cc=glider@google.com \
--cc=kasan-dev@googlegroups.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=llvm@lists.linux.dev \
--cc=longman@redhat.com \
--cc=mark.rutland@arm.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).