linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
@ 2020-01-21 15:15 Qian Cai
  2020-01-21 15:19 ` Marco Elver
  0 siblings, 1 reply; 14+ messages in thread
From: Qian Cai @ 2020-01-21 15:15 UTC (permalink / raw)
  To: tglx, mingo, bp
  Cc: dave.hansen, luto, peterz, elver, x86, linux-kernel, Qian Cai

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 <elver@google.com>
Signed-off-by: Qian Cai <cai@lca.pw>
---
 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)


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:15 [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install Qian Cai
@ 2020-01-21 15:19 ` Marco Elver
  2020-01-21 15:28   ` Borislav Petkov
  0 siblings, 1 reply; 14+ messages in thread
From: Marco Elver @ 2020-01-21 15:19 UTC (permalink / raw)
  To: Qian Cai
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML

On Tue, 21 Jan 2020 at 16:15, Qian Cai <cai@lca.pw> 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 <elver@google.com>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  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)
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:19 ` Marco Elver
@ 2020-01-21 15:28   ` Borislav Petkov
  2020-01-21 15:33     ` Qian Cai
  0 siblings, 1 reply; 14+ messages in thread
From: Borislav Petkov @ 2020-01-21 15:28 UTC (permalink / raw)
  To: Qian Cai
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:28   ` Borislav Petkov
@ 2020-01-21 15:33     ` Qian Cai
  2020-01-21 15:36       ` Marco Elver
  0 siblings, 1 reply; 14+ messages in thread
From: Qian Cai @ 2020-01-21 15:33 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML



> On Jan 21, 2020, at 10:28 AM, Borislav Petkov <bp@alien8.de> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:33     ` Qian Cai
@ 2020-01-21 15:36       ` Marco Elver
  2020-01-21 15:45         ` Borislav Petkov
  0 siblings, 1 reply; 14+ messages in thread
From: Marco Elver @ 2020-01-21 15:36 UTC (permalink / raw)
  To: Qian Cai
  Cc: Borislav Petkov, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML

On Tue, 21 Jan 2020 at 16:33, Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Jan 21, 2020, at 10:28 AM, Borislav Petkov <bp@alien8.de> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:36       ` Marco Elver
@ 2020-01-21 15:45         ` Borislav Petkov
  2020-01-21 20:21           ` Qian Cai
  2020-01-22  8:46           ` Peter Zijlstra
  0 siblings, 2 replies; 14+ messages in thread
From: Borislav Petkov @ 2020-01-21 15:45 UTC (permalink / raw)
  To: Marco Elver, Qian Cai
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, Andy Lutomirski,
	Peter Zijlstra, the arch/x86 maintainers, LKML

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:45         ` Borislav Petkov
@ 2020-01-21 20:21           ` Qian Cai
  2020-01-21 22:18             ` Borislav Petkov
  2020-01-22  8:46           ` Peter Zijlstra
  1 sibling, 1 reply; 14+ messages in thread
From: Qian Cai @ 2020-01-21 20:21 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML



> On Jan 21, 2020, at 10:45 AM, Borislav Petkov <bp@alien8.de> 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? 

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 20:21           ` Qian Cai
@ 2020-01-21 22:18             ` Borislav Petkov
  2020-01-21 23:30               ` Marco Elver
  2020-01-22  0:34               ` Qian Cai
  0 siblings, 2 replies; 14+ messages in thread
From: Borislav Petkov @ 2020-01-21 22:18 UTC (permalink / raw)
  To: Qian Cai
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 22:18             ` Borislav Petkov
@ 2020-01-21 23:30               ` Marco Elver
  2020-01-22  0:34               ` Qian Cai
  1 sibling, 0 replies; 14+ messages in thread
From: Marco Elver @ 2020-01-21 23:30 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Qian Cai, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML

On Tue, 21 Jan 2020 at 23:18, Borislav Petkov <bp@alien8.de> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 22:18             ` Borislav Petkov
  2020-01-21 23:30               ` Marco Elver
@ 2020-01-22  0:34               ` Qian Cai
  1 sibling, 0 replies; 14+ messages in thread
From: Qian Cai @ 2020-01-22  0:34 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML



> On Jan 21, 2020, at 5:18 PM, Borislav Petkov <bp@alien8.de> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-21 15:45         ` Borislav Petkov
  2020-01-21 20:21           ` Qian Cai
@ 2020-01-22  8:46           ` Peter Zijlstra
  2020-01-23  2:15             ` Qian Cai
  1 sibling, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2020-01-22  8:46 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Marco Elver, Qian Cai, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, the arch/x86 maintainers, LKML

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-22  8:46           ` Peter Zijlstra
@ 2020-01-23  2:15             ` Qian Cai
  2020-01-23  8:16               ` Borislav Petkov
  0 siblings, 1 reply; 14+ messages in thread
From: Qian Cai @ 2020-01-23  2:15 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Borislav Petkov, Marco Elver, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, Andy Lutomirski, the arch/x86 maintainers, LKML



> On Jan 22, 2020, at 3:46 AM, Peter Zijlstra <peterz@infradead.org> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
  2020-01-23  2:15             ` Qian Cai
@ 2020-01-23  8:16               ` Borislav Petkov
  0 siblings, 0 replies; 14+ messages in thread
From: Borislav Petkov @ 2020-01-23  8:16 UTC (permalink / raw)
  To: Qian Cai
  Cc: Peter Zijlstra, Marco Elver, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, Andy Lutomirski, the arch/x86 maintainers, LKML

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install
@ 2020-01-21 15:50 Qian Cai
  0 siblings, 0 replies; 14+ messages in thread
From: Qian Cai @ 2020-01-21 15:50 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Marco Elver, Thomas Gleixner, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, the arch/x86 maintainers, LKML



> On Jan 21, 2020, at 10:45 AM, Borislav Petkov <bp@alien8.de> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-01-23  8:16 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-21 15:15 [PATCH -next] x86/mm/pat: silence a data race in cpa_4k_install Qian Cai
2020-01-21 15:19 ` Marco Elver
2020-01-21 15:28   ` Borislav Petkov
2020-01-21 15:33     ` Qian Cai
2020-01-21 15:36       ` Marco Elver
2020-01-21 15:45         ` Borislav Petkov
2020-01-21 20:21           ` Qian Cai
2020-01-21 22:18             ` Borislav Petkov
2020-01-21 23:30               ` Marco Elver
2020-01-22  0:34               ` Qian Cai
2020-01-22  8:46           ` Peter Zijlstra
2020-01-23  2:15             ` Qian Cai
2020-01-23  8:16               ` Borislav Petkov
2020-01-21 15:50 Qian Cai

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