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. Fix it by
adding a pair of READ_ONCE() and WRITE_ONCE().
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..31e4a73ae70e 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++;
+ WRITE_ONCE(cpa_4k_install, READ_ONCE(cpa_4k_install) + 1);
}
static inline void cpa_inc_lp_sameprot(int level)
--
2.21.0 (Apple Git-122.2)
On Mon, Jan 20, 2020 at 11:12:00PM -0500, Qian Cai wrote:
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20823392f4f2..31e4a73ae70e 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++;
> + WRITE_ONCE(cpa_4k_install, READ_ONCE(cpa_4k_install) + 1);
> }
>
> static inline void cpa_inc_lp_sameprot(int level)
> --
Fix a data race, says your subject?
If it had to be honest, it probably should say "Make the code ugly
because the next tool can't handle it".
Frankly, I'm not a fan of all this "change the kernel to fix the tool"
attitude.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, 21 Jan 2020 at 08:27, Borislav Petkov <[email protected]> wrote:
>
> On Mon, Jan 20, 2020 at 11:12:00PM -0500, Qian Cai wrote:
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index 20823392f4f2..31e4a73ae70e 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++;
> > + WRITE_ONCE(cpa_4k_install, READ_ONCE(cpa_4k_install) + 1);
As I said in my email that you also copied to the message, this is
just a stats counter. For the general case, I think we reached
consensus that such accesses should intentionally remain data races:
https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com/T/#u
Either you can use the data_race() macro, making this
'data_race(cpa_4k_install++)' -- this effectively documents the
intentional data race --
or just blacklist the entire file by putting
KCSAN_SANITIZE_set_memory.o := n
into the Makefile.
[ Note that there are 2 more ways to blacklist:
- __no_kcsan function attribute, for blacklisting entire functions.
- KCSAN_SANITIZE :=n in the Makefile, blacklisting all compilation
units in the Makefile. ]
I leave it to you what makes more sense. I don't know if there are
other data races lurking here, since cpa_4k_install is not the only
stats counter.
> > }
> >
> > static inline void cpa_inc_lp_sameprot(int level)
> > --
>
> Fix a data race, says your subject?
>
> If it had to be honest, it probably should say "Make the code ugly
> because the next tool can't handle it".
>
> Frankly, I'm not a fan of all this "change the kernel to fix the tool"
> attitude.
I commented on better ways dealing with this, since this is just stats counting.
Thanks,
-- Marco
On Tue, Jan 21, 2020 at 09:19:18AM +0100, Marco Elver wrote:
> As I said in my email that you also copied to the message, this is
> just a stats counter. For the general case, I think we reached
> consensus that such accesses should intentionally remain data races:
> https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com/T/#u
Yap, I agree with Linus on the legibility aspect.
> Either you can use the data_race() macro, making this
> 'data_race(cpa_4k_install++)' -- this effectively documents the
> intentional data race --
>
> or just blacklist the entire file by putting
> KCSAN_SANITIZE_set_memory.o := n
> into the Makefile.
>
> [ Note that there are 2 more ways to blacklist:
> - __no_kcsan function attribute, for blacklisting entire functions.
> - KCSAN_SANITIZE :=n in the Makefile, blacklisting all compilation
> units in the Makefile. ]
Do we have all those official methods how to make KCSAN happy,
documented somewhere?
> I leave it to you what makes more sense. I don't know if there are
> other data races lurking here, since cpa_4k_install is not the only
> stats counter.
In this particular case and if it were me, I'd prefer the __no_kcsan
function attribute because it is kept outside of the function body. But
I can't find __no_kcsan in current tip:
$ git grep __no_kcsan
.h:204:static __no_kcsan_or_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
include/linux/compiler.h:215:# define __no_kcsan_or_inline __no_sanitize_thread notrace __maybe_unused
include/linux/compiler.h:216:# define __no_sanitize_or_inline __no_kcsan_or_inline
include/linux/compiler.h:218:# define __no_kcsan_or_inline __always_inline
include/linux/compiler.h:225:static __no_kcsan_or_inline
include/linux/compiler.h:238:static __no_kcsan_or_inline
just this "glued together" thing __no_kcsan_or_inline.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, 21 Jan 2020 at 10:16, Borislav Petkov <[email protected]> wrote:
>
> On Tue, Jan 21, 2020 at 09:19:18AM +0100, Marco Elver wrote:
> > As I said in my email that you also copied to the message, this is
> > just a stats counter. For the general case, I think we reached
> > consensus that such accesses should intentionally remain data races:
> > https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com/T/#u
>
> Yap, I agree with Linus on the legibility aspect.
>
> > Either you can use the data_race() macro, making this
> > 'data_race(cpa_4k_install++)' -- this effectively documents the
> > intentional data race --
> >
> > or just blacklist the entire file by putting
> > KCSAN_SANITIZE_set_memory.o := n
> > into the Makefile.
> >
> > [ Note that there are 2 more ways to blacklist:
> > - __no_kcsan function attribute, for blacklisting entire functions.
> > - KCSAN_SANITIZE :=n in the Makefile, blacklisting all compilation
> > units in the Makefile. ]
>
> Do we have all those official methods how to make KCSAN happy,
> documented somewhere?
Yes, it's in Documentation/dev-tools/kcsan.rst. I sent a patch last
month, which is in the -rcu tree:
http://lkml.kernel.org/r/[email protected]
> > I leave it to you what makes more sense. I don't know if there are
> > other data races lurking here, since cpa_4k_install is not the only
> > stats counter.
>
> In this particular case and if it were me, I'd prefer the __no_kcsan
> function attribute because it is kept outside of the function body. But
> I can't find __no_kcsan in current tip:
>
> $ git grep __no_kcsan
> .h:204:static __no_kcsan_or_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
> include/linux/compiler.h:215:# define __no_kcsan_or_inline __no_sanitize_thread notrace __maybe_unused
> include/linux/compiler.h:216:# define __no_sanitize_or_inline __no_kcsan_or_inline
> include/linux/compiler.h:218:# define __no_kcsan_or_inline __always_inline
> include/linux/compiler.h:225:static __no_kcsan_or_inline
> include/linux/compiler.h:238:static __no_kcsan_or_inline
>
> just this "glued together" thing __no_kcsan_or_inline.
As far as I can tell it's not yet in -tip, but only in -rcu (and
-next). I believe it should be in -tip soon.
Thanks,
-- Marco
On Tue, Jan 21, 2020 at 09:19:18AM +0100, Marco Elver wrote:
> On Tue, 21 Jan 2020 at 08:27, Borislav Petkov <[email protected]> wrote:
> >
> > On Mon, Jan 20, 2020 at 11:12:00PM -0500, Qian Cai wrote:
> > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > index 20823392f4f2..31e4a73ae70e 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++;
> > > + WRITE_ONCE(cpa_4k_install, READ_ONCE(cpa_4k_install) + 1);
>
> As I said in my email that you also copied to the message, this is
> just a stats counter. For the general case, I think we reached
> consensus that such accesses should intentionally remain data races:
> https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com/T/#u
>
> Either you can use the data_race() macro, making this
> 'data_race(cpa_4k_install++)' -- this effectively documents the
> intentional data race --
Yes, that sounds useful.
But this patch, as presented, is just plain wrong. It doesn't fix
anything.