LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Marco Elver <elver@google.com>
To: Qian Cai <cai@lca.pw>
Cc: "Paul E. McKenney" <paulmck@kernel.org>,
	Alexander Potapenko <glider@google.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Dmitriy Vyukov <dvyukov@google.com>,
	kasan-dev <kasan-dev@googlegroups.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH -rcu] kcsan: Make KCSAN compatible with lockdep
Date: Fri, 17 Jan 2020 23:51:48 +0100	[thread overview]
Message-ID: <CANpmjNPdfB=hrcXJbPzdisxnBUZW3JEK9UbTpTy+a20b=6OdJg@mail.gmail.com> (raw)
In-Reply-To: <3760F60F-4133-4FE1-9A4C-F335A8230285@lca.pw>

On Fri, 17 Jan 2020 at 17:59, Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Jan 17, 2020, at 11:40 AM, Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > True enough, but even if we reach the nirvana state where there is general
> > agreement on what constitutes a data race in need of fixing and KCSAN
> > faithfully checks based on that data-race definition, we need to handle
> > the case where someone introduces a bug that results in a destructive
> > off-CPU access to a per-CPU variable, which is exactly the sort of thing
> > that KCSAN is supposed to detect.  But suppose that this variable is
> > frequently referenced from functions that are inlined all over the place.
> >
> > Then that one bug might result in huge numbers of data-race reports in
> > a very short period of time, especially on a large system.
>
> It sounds like the case with debug_pagealloc where it prints a spam of those, and then the system is just dead.
>
> [   28.992752][  T394] Reported by Kernel Concurrency Sanitizer on:
> [   28.992752][  T394] CPU: 0 PID: 394 Comm: pgdatinit0 Not tainted 5.5.0-rc6-next-20200115+ #3
> [   28.992752][  T394] Hardware name: HP ProLiant XL230a Gen9/ProLiant XL230a Gen9, BIOS U13 01/22/2018
> [   28.992752][  T394] ===============================================================
> [   28.992752][  T394] ==================================================================
> [   28.992752][  T394] BUG: KCSAN: data-race in __change_page_attr / __change_page_attr
> [   28.992752][  T394]
> [   28.992752][  T394] read to 0xffffffffa01a6de0 of 8 bytes by task 395 on cpu 16:
> [   28.992752][  T394]  __change_page_attr+0xe81/0x1620
> [   28.992752][  T394]  __change_page_attr_set_clr+0xde/0x4c0
> [   28.992752][  T394]  __set_pages_np+0xcc/0x100
> [   28.992752][  T394]  __kernel_map_pages+0xd6/0xdb
> [   28.992752][  T394]  __free_pages_ok+0x1a8/0x730
> [   28.992752][  T394]  __free_pages+0x51/0x90
> [   28.992752][  T394]  __free_pages_core+0x1c7/0x2c0
> [   28.992752][  T394]  deferred_free_range+0x59/0x8f
> [   28.992752][  T394]  deferred_init_max21d
> [   28.992752][  T394]  deferred_init_memmap+0x14a/0x1c1
> [   28.992752][  T394]  kthread+0x1e0/0x200
> [   28.992752][  T394]  ret_from_fork+0x3a/0x50
> [   28.992752][  T394]
> [   28.992752][  T394] write to 0xffffffffa01a6de0 of 8 bytes by task 394 on cpu 0:
> [   28.992752][  T394]  __change_page_attr+0xe9c/0x1620
> [   28.992752][  T394]  __change_page_attr_set_clr+0xde/0x4c0
> [   28.992752][  T394]  __set_pages_np+0xcc/0x100
> [   28.992752][  T394]  __kernel_map_pages+0xd6/0xdb
> [   28.992752][  T394]  __free_pages_ok+0x1a8/0x730
> [   28.992752][  T394]  __free_pages+0x51/0x90
> [   28.992752][  T394]  __free_pages_core+0x1c7/0x2c0
> [   28.992752][  T394]  deferred_free_range+0x59/0x8f
> [   28.992752][  T394]  deferred_init_maxorder+0x1d6/0x21d
> [   28.992752][  T394]  deferred_init_memmap+0x14a/0x1c1
> [   28.992752][  T394]  kthread+0x1e0/0x200
> [   28.992752][  T394]  ret_from_fork+0x3a/0x50
>
> It point out to this,
>
>                 pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
>                 pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
>
>                 cpa_inc_4k_install();
>                 /* Hand in lpsize = 0 to enforce the protection mechanism */
>                 new_prot = static_protections(new_prot, address, pfn, 1, 0,
>                                               CPA_PROTECT);
>
> In static_protections(),
>
>         /*
>          * There is no point in checking RW/NX conflicts when the requested
>          * mapping is setting the page !PRESENT.
>          */
>         if (!(pgprot_val(prot) & _PAGE_PRESENT))
>                 return prot;
>
> Is there a data race there?

Yes. I was finally able to reproduce this data race on linux-next (my
system doesn't crash though, maybe not enough cores?). Here is a trace
with line numbers:

read to 0xffffffffaa59a000 of 8 bytes by interrupt on cpu 7:
 cpa_inc_4k_install arch/x86/mm/pat/set_memory.c:131 [inline]
 __change_page_attr+0x10cf/0x1840 arch/x86/mm/pat/set_memory.c:1514
 __change_page_attr_set_clr+0xce/0x490 arch/x86/mm/pat/set_memory.c:1636
 __set_pages_np+0xc4/0xf0 arch/x86/mm/pat/set_memory.c:2148
 __kernel_map_pages+0xb0/0xc8 arch/x86/mm/pat/set_memory.c:2178
 kernel_map_pages include/linux/mm.h:2719 [inline]
<snip>

write to 0xffffffffaa59a000 of 8 bytes by task 1 on cpu 6:
 cpa_inc_4k_install arch/x86/mm/pat/set_memory.c:131 [inline]
 __change_page_attr+0x10ea/0x1840 arch/x86/mm/pat/set_memory.c:1514
 __change_page_attr_set_clr+0xce/0x490 arch/x86/mm/pat/set_memory.c:1636
 __set_pages_p+0xc4/0xf0 arch/x86/mm/pat/set_memory.c:2129
 __kernel_map_pages+0x2e/0xc8 arch/x86/mm/pat/set_memory.c:2176
 kernel_map_pages include/linux/mm.h:2719 [inline]
<snip>

Both accesses are due to the same "cpa_4k_install++" in
cpa_inc_4k_install. Now you can see that a data race here could be
potentially undesirable: depending on compiler optimizations or how
x86 executes a non-LOCK'd increment, you may lose increments, corrupt
the counter, etc. Since this counter only seems to be used for
printing some stats, this data race itself is unlikely to cause harm
to the system though.

Thanks,
-- Marco

  reply	other threads:[~2020-01-17 22:52 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14 12:49 Marco Elver
2020-01-14 17:24 ` Alexander Potapenko
2020-01-15 16:26   ` Marco Elver
2020-01-15 16:37     ` Paul E. McKenney
2020-01-16  3:39       ` Qian Cai
2020-01-16  4:37         ` Qian Cai
2020-01-16 17:40           ` Paul E. McKenney
2020-01-16 17:50             ` Qian Cai
2020-01-16 17:55               ` Qian Cai
2020-01-16 18:57         ` Marco Elver
2020-01-17 16:40           ` Paul E. McKenney
2020-01-17 16:59             ` Qian Cai
2020-01-17 22:51               ` Marco Elver [this message]
2020-01-17 22:54             ` Marco Elver
2020-01-17 23:23               ` Paul E. McKenney

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='CANpmjNPdfB=hrcXJbPzdisxnBUZW3JEK9UbTpTy+a20b=6OdJg@mail.gmail.com' \
    --to=elver@google.com \
    --cc=andreyknvl@google.com \
    --cc=cai@lca.pw \
    --cc=dvyukov@google.com \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH -rcu] kcsan: Make KCSAN compatible with lockdep' \
    /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

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).