On Mon, 6 Dec 2021 at 06:04, Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> Hi,
>
> On Tue, Nov 30, 2021 at 12:44:16PM +0100, Marco Elver wrote:
> > Also show the location the access was reordered to. An example report:
> >
> > | ==================================================================
> > | BUG: KCSAN: data-race in test_kernel_wrong_memorder / test_kernel_wrong_memorder
> > |
> > | read-write to 0xffffffffc01e61a8 of 8 bytes by task 2311 on cpu 5:
> > | test_kernel_wrong_memorder+0x57/0x90
> > | access_thread+0x99/0xe0
> > | kthread+0x2ba/0x2f0
> > | ret_from_fork+0x22/0x30
> > |
> > | read-write (reordered) to 0xffffffffc01e61a8 of 8 bytes by task 2310 on cpu 7:
> > | test_kernel_wrong_memorder+0x57/0x90
> > | access_thread+0x99/0xe0
> > | kthread+0x2ba/0x2f0
> > | ret_from_fork+0x22/0x30
> > | |
> > | +-> reordered to: test_kernel_wrong_memorder+0x80/0x90
> > |
>
> Should this be "reordered from" instead of "reordered to"? For example,
> if the following case needs a smp_mb() between write to A and write to
> B, I think currently it will report as follow:
>
> foo() {
> WRITE_ONCE(A, 1); // let's say A's address is 0xaaaa
> bar() {
> WRITE_ONCE(B, 1); // Assume B's address is 0xbbbb
> // KCSAN find the problem here
> }
> }
>
> <report>
> | write (reordered) to 0xaaaa of ...:
> | bar+0x... // address of the write to B
> | foo+0x... // address of the callsite to bar()
> | ...
> | |
> | +-> reordered to: foo+0x... // address of the write to A
>
> But since the access reported here is the write to A, so it's a
> "reordered from" instead of "reordered to"?
Perhaps I could have commented on this in the commit message to avoid
the confusion, but per its updated comment replace_stack_entry()
"skips to the first entry that matches the function of @ip, and then
replaces that entry with @ip, returning the entries to skip with
@replaced containing the replaced entry."
When a reorder_access is set up, the ip to it is stored, which is
what's passed to @ip of replace_stack_entry(). It effectively swaps
the top frame where the race occurred with where the original access
happened. This all works because the runtime is careful to only keep
reorder_accesses valid until the original function where it occurred
is left.
So in your above example you need to swap "reordered to" and the top
frame of the stack trace.
The implementation is a little trickier of course, but I really wanted
the main stack trace to look like any other non-reordered access,
which starts from the original access, and only have the "reordered
to" location be secondary information.
The foundation for doing this this was put in place here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6c65eb75686f
Thanks,
-- Marco
