Macro Elver mentioned,
"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."
This will generate a lot of noise on a debug kernel with debug_pagealloc
with KCSAN enabled which could render the system unusable. Silence it by
using the data_race() macro.
Suggested-by: Macro Elver <[email protected]>
Signed-off-by: Qian Cai <[email protected]>
---
arch/x86/mm/pat/set_memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 20823392f4f2..a5c35e57905e 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -128,7 +128,7 @@ static inline void cpa_inc_2m_checked(void)
static inline void cpa_inc_4k_install(void)
{
- cpa_4k_install++;
+ data_race(cpa_4k_install++);
}
static inline void cpa_inc_lp_sameprot(int level)
--
2.21.0 (Apple Git-122.2)
On Tue, 21 Jan 2020 at 16:15, Qian Cai <[email protected]> wrote:
>
> Macro Elver mentioned,
>
> "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."
Thank you for the patch!
Could you remove the verbatim copy of my email? Maybe something like:
"Increments to cpa_4k_install may happen concurrently, as detected by KCSAN:
<....... the stack traces ......>
Since the counter is only used to count stats, a data race will not be
harmful, thus we mark it as an intentional data race with the
'data_race()' macro.
Otherwise, this may generate a lot of noise on a debug kernel with
debug_pagealloc
with KCSAN enabled which could render the system unusable."
Thanks,
-- Marco
> This will generate a lot of noise on a debug kernel with debug_pagealloc
> with KCSAN enabled which could render the system unusable. Silence it by
> using the data_race() macro.
>
> Suggested-by: Macro Elver <[email protected]>
> Signed-off-by: Qian Cai <[email protected]>
> ---
> arch/x86/mm/pat/set_memory.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20823392f4f2..a5c35e57905e 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -128,7 +128,7 @@ static inline void cpa_inc_2m_checked(void)
>
> static inline void cpa_inc_4k_install(void)
> {
> - cpa_4k_install++;
> + data_race(cpa_4k_install++);
> }
>
> static inline void cpa_inc_lp_sameprot(int level)
> --
> 2.21.0 (Apple Git-122.2)
>
On Tue, Jan 21, 2020 at 04:19:22PM +0100, Marco Elver wrote:
> Could you remove the verbatim copy of my email? Maybe something like:
>
> "Increments to cpa_4k_install may happen concurrently, as detected by KCSAN:
>
> <....... the stack traces ......>
... and drop the stack traces and fix your subject to say what you're
actually "fixing".
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> On Jan 21, 2020, at 10:28 AM, Borislav Petkov <[email protected]> wrote:
>
> On Tue, Jan 21, 2020 at 04:19:22PM +0100, Marco Elver wrote:
>> Could you remove the verbatim copy of my email? Maybe something like:
>>
>> "Increments to cpa_4k_install may happen concurrently, as detected by KCSAN:
>>
>> <....... the stack traces ......>
>
> ... and drop the stack traces and fix your subject to say what you're
> actually "fixing".
Does this title work for you?
x86/mm/pat: silence an data race for KCSAN
On Tue, 21 Jan 2020 at 16:33, Qian Cai <[email protected]> wrote:
>
>
>
> > On Jan 21, 2020, at 10:28 AM, Borislav Petkov <[email protected]> wrote:
> >
> > On Tue, Jan 21, 2020 at 04:19:22PM +0100, Marco Elver wrote:
> >> Could you remove the verbatim copy of my email? Maybe something like:
> >>
> >> "Increments to cpa_4k_install may happen concurrently, as detected by KCSAN:
> >>
> >> <....... the stack traces ......>
> >
> > ... and drop the stack traces and fix your subject to say what you're
> > actually "fixing".
>
> Does this title work for you?
>
> x86/mm/pat: silence an data race for KCSAN
Isn't the intent "x86/mm/pat: Mark intentional data race" ? The fact
that KCSAN no longer shows the warning is a side-effect. At least
that's how I see it.
Thanks,
-- Marco
On Tue, Jan 21, 2020 at 04:36:49PM +0100, Marco Elver wrote:
> Isn't the intent "x86/mm/pat: Mark intentional data race" ? The fact
> that KCSAN no longer shows the warning is a side-effect. At least
> that's how I see it.
Perhaps because you've been dealing with KCSAN for so long. :-)
The main angle here, IMO, is that this "fix" is being done solely for
KCSAN. Or is there another reason to "fix" intentional data races? At
least I don't see one. And the text says
"This will generate a lot of noise on a debug kernel with
debug_pagealloc with KCSAN enabled which could render the system
unusable."
So yes, I think it should say something about making KCSAN happy.
Oh, and while at it I'd prefer it if it did the __no_kcsan function
annotation instead of the data_race() thing.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> On Jan 21, 2020, at 10:45 AM, Borislav Petkov <[email protected]> wrote:
>
> Perhaps because you've been dealing with KCSAN for so long. :-)
>
> The main angle here, IMO, is that this "fix" is being done solely for
> KCSAN. Or is there another reason to "fix" intentional data races? At
> least I don't see one. And the text says
>
> "This will generate a lot of noise on a debug kernel with
> debug_pagealloc with KCSAN enabled which could render the system
> unusable."
>
> So yes, I think it should say something about making KCSAN happy.
>
> Oh, and while at it I'd prefer it if it did the __no_kcsan function
> annotation instead of the data_race() thing.
Or the patch title could be “play KCSAN well with debug_pagealloc”?
I am fine with __no_kcsan as well. I just need to retest the whole thing first.
> On Jan 21, 2020, at 10:45 AM, Borislav Petkov <[email protected]> wrote:
>
> On Tue, Jan 21, 2020 at 04:36:49PM +0100, Marco Elver wrote:
>> Isn't the intent "x86/mm/pat: Mark intentional data race" ? The fact
>> that KCSAN no longer shows the warning is a side-effect. At least
>> that's how I see it.
>
> Perhaps because you've been dealing with KCSAN for so long. :-)
>
> The main angle here, IMO, is that this "fix" is being done solely for
> KCSAN. Or is there another reason to "fix" intentional data races? At
> least I don't see one. And the text says
>
> "This will generate a lot of noise on a debug kernel with
> debug_pagealloc with KCSAN enabled which could render the system
> unusable."
>
> So yes, I think it should say something about making KCSAN happy.
>
> Oh, and while at it I'd prefer it if it did the __no_kcsan function
> annotation instead of the data_race() thing.
Actually "__no_kcsan" does not work because I have
CONFIG_OPTIMIZE_INLINING=y (GCC 8.3.1) here, so it has to be,
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 20823392f4f2..fabbf8a33b7f 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -126,7 +126,7 @@ static inline void cpa_inc_2m_checked(void)
cpa_2m_checked++;
}
-static inline void cpa_inc_4k_install(void)
+static inline void __no_kcsan_or_inline cpa_inc_4k_install(void)
{
cpa_4k_install++;
}
Are you fine with it or data_race() looks better?
On Tue, Jan 21, 2020 at 03:21:35PM -0500, Qian Cai wrote:
> Actually "__no_kcsan" does not work because I have
Why, because KCSAN conflicts with inlining? I'm looking at the comment
over __no_kasan_or_inline.
> CONFIG_OPTIMIZE_INLINING=y (GCC 8.3.1) here, so it has to be,
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20823392f4f2..fabbf8a33b7f 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -126,7 +126,7 @@ static inline void cpa_inc_2m_checked(void)
> cpa_2m_checked++;
> }
>
> -static inline void cpa_inc_4k_install(void)
> +static inline void __no_kcsan_or_inline cpa_inc_4k_install(void)
> {
> cpa_4k_install++;
> }
>
> Are you fine with it or data_race() looks better?
This one looks marginally better because the annotation is still outside
of the function, so to speak.
Btw, looking at the other "inc" CPA statistics functions there, does it
mean that for KCSAN they all need to be annotated now too?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, 21 Jan 2020 at 23:18, Borislav Petkov <[email protected]> wrote:
>
> On Tue, Jan 21, 2020 at 03:21:35PM -0500, Qian Cai wrote:
> > Actually "__no_kcsan" does not work because I have
>
> Why, because KCSAN conflicts with inlining? I'm looking at the comment
> over __no_kasan_or_inline.
Rather a bug in GCC. AFAIK it is GCC <9 ignoring
__attribute__((no_sanitize_*)) for functions that it decides to
inline. Somewhere GCC loses the attribute when inlined, resulting in
still emitting instrumentation calls. __no_kcsan_or_inline works
around it by making such functions noinline. GCC also emits an error
if you try to combine __always_inline and
__attribute__((no_sanitize_*)).
For these reasons we sadly need __no_kcsan_or_inline (similarly we
need __no_kasan_or_inline for KASAN for the same reasons). I hope this
will look a bit nicer once we move past GCC<9.
> > CONFIG_OPTIMIZE_INLINING=y (GCC 8.3.1) here, so it has to be,
> >
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index 20823392f4f2..fabbf8a33b7f 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -126,7 +126,7 @@ static inline void cpa_inc_2m_checked(void)
> > cpa_2m_checked++;
> > }
> >
> > -static inline void cpa_inc_4k_install(void)
> > +static inline void __no_kcsan_or_inline cpa_inc_4k_install(void)
It should be 'static __no_kcsan_or_inline void
cpa_inc_4k_install(void)', since __no_kcsan_or_inline provides
__always_inline on non-KCSAN builds.
Thanks,
-- Marco
> > {
> > cpa_4k_install++;
> > }
> >
> > Are you fine with it or data_race() looks better?
>
> This one looks marginally better because the annotation is still outside
> of the function, so to speak.
>
> Btw, looking at the other "inc" CPA statistics functions there, does it
> mean that for KCSAN they all need to be annotated now too?
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
> On Jan 21, 2020, at 5:18 PM, Borislav Petkov <[email protected]> wrote:
>
> Btw, looking at the other "inc" CPA statistics functions there, does it
> mean that for KCSAN they all need to be annotated now too?
I don’t know. KCSAN never trigger any warnings for other CPA places yet.
On Tue, Jan 21, 2020 at 04:45:28PM +0100, Borislav Petkov wrote:
> On Tue, Jan 21, 2020 at 04:36:49PM +0100, Marco Elver wrote:
> > Isn't the intent "x86/mm/pat: Mark intentional data race" ? The fact
> > that KCSAN no longer shows the warning is a side-effect. At least
> > that's how I see it.
>
> Perhaps because you've been dealing with KCSAN for so long. :-)
>
> The main angle here, IMO, is that this "fix" is being done solely for
> KCSAN. Or is there another reason to "fix" intentional data races? At
> least I don't see one. And the text says
Documentation. It is a clear and concise marker of intent. Unintended
data races are bad.
Also, we've been adding annotations to the kernel source forever,
sparse, lockdep, etc.. now KCSAN. All we have to do is make sure they're
minimally invasive, and in that regard the date_race() marker is spot on
IMO.
> On Jan 22, 2020, at 3:46 AM, Peter Zijlstra <[email protected]> wrote:
>
> Documentation. It is a clear and concise marker of intent. Unintended
> data races are bad.
>
> Also, we've been adding annotations to the kernel source forever,
> sparse, lockdep, etc.. now KCSAN. All we have to do is make sure they're
> minimally invasive, and in that regard the date_race() marker is spot on
> IMO.
Okay, so which way should we move forward with this then? Borislav liked __no_kasan_or_inline and Peter liked data_race(). I personally like data_race() more because it has nothing to do with the GCC bug, but I realized my opinion has little weight here.
On Wed, Jan 22, 2020 at 09:15:07PM -0500, Qian Cai wrote:
> Okay, so which way should we move forward with this then? Borislav
> liked __no_kasan_or_inline and Peter liked data_race(). I personally
> like data_race() more because it has nothing to do with the GCC bug,
> but I realized my opinion has little weight here.
Do the data_race() thing, pls.
But do send after the merge window because latter should open next week
and we all will be busy and not review/apply new stuff.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette