linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -next] x86/mm/pat: mark an intentional data race
@ 2020-02-10 14:10 Qian Cai
  2020-03-06 14:16 ` Qian Cai
  2020-03-11 16:17 ` Borislav Petkov
  0 siblings, 2 replies; 4+ messages in thread
From: Qian Cai @ 2020-02-10 14:10 UTC (permalink / raw)
  To: tglx, mingo, bp
  Cc: dave.hansen, luto, peterz, elver, x86, linux-kernel, Qian Cai

cpa_4k_install could be accessed concurrently as noticed by KCSAN,

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. A data race here could be potentially undesirable:
depending on compiler optimizations or how x86 executes a non-LOCK'd
increment, it 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. Thus, mark this
intentional data race using the data_race() marco.

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 c4aedd00c1ba..ea0b6df950ee 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)
-- 
1.8.3.1


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

* Re: [PATCH -next] x86/mm/pat: mark an intentional data race
  2020-02-10 14:10 [PATCH -next] x86/mm/pat: mark an intentional data race Qian Cai
@ 2020-03-06 14:16 ` Qian Cai
  2020-03-11 16:17 ` Borislav Petkov
  1 sibling, 0 replies; 4+ messages in thread
From: Qian Cai @ 2020-03-06 14:16 UTC (permalink / raw)
  To: tglx, mingo, bp; +Cc: dave.hansen, luto, peterz, elver, x86, linux-kernel

On Mon, 2020-02-10 at 09:10 -0500, Qian Cai wrote:
> cpa_4k_install could be accessed concurrently as noticed by KCSAN,
> 
> 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. A data race here could be potentially undesirable:
> depending on compiler optimizations or how x86 executes a non-LOCK'd
> increment, it 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. Thus, mark this
> intentional data race using the data_race() marco.

Borislav or any other maintainers, can you take a look at this patch when you
had a chance?

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

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

* Re: [PATCH -next] x86/mm/pat: mark an intentional data race
  2020-02-10 14:10 [PATCH -next] x86/mm/pat: mark an intentional data race Qian Cai
  2020-03-06 14:16 ` Qian Cai
@ 2020-03-11 16:17 ` Borislav Petkov
  2020-03-11 19:04   ` Paul E. McKenney
  1 sibling, 1 reply; 4+ messages in thread
From: Borislav Petkov @ 2020-03-11 16:17 UTC (permalink / raw)
  To: Qian Cai
  Cc: tglx, mingo, dave.hansen, luto, peterz, elver, x86, linux-kernel,
	Paul E. McKenney

+ Paul.

On Mon, Feb 10, 2020 at 09:10:16AM -0500, Qian Cai wrote:
> cpa_4k_install could be accessed concurrently as noticed by KCSAN,
> 
> 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. A data race here could be potentially undesirable:
> depending on compiler optimizations or how x86 executes a non-LOCK'd
> increment, it 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. Thus, mark this
> intentional data race using the data_race() marco.
> 
> 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 c4aedd00c1ba..ea0b6df950ee 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)
> -- 

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette


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

* Re: [PATCH -next] x86/mm/pat: mark an intentional data race
  2020-03-11 16:17 ` Borislav Petkov
@ 2020-03-11 19:04   ` Paul E. McKenney
  0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2020-03-11 19:04 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Qian Cai, tglx, mingo, dave.hansen, luto, peterz, elver, x86,
	linux-kernel

On Wed, Mar 11, 2020 at 05:17:56PM +0100, Borislav Petkov wrote:
> + Paul.
> 
> On Mon, Feb 10, 2020 at 09:10:16AM -0500, Qian Cai wrote:
> > cpa_4k_install could be accessed concurrently as noticed by KCSAN,
> > 
> > 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. A data race here could be potentially undesirable:
> > depending on compiler optimizations or how x86 executes a non-LOCK'd
> > increment, it 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. Thus, mark this
> > intentional data race using the data_race() marco.
> > 
> > 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 c4aedd00c1ba..ea0b6df950ee 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)
> > -- 
> 
> Acked-by: Borislav Petkov <bp@suse.de>

Applied, thank you both!

							Thanx, Paul

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

end of thread, other threads:[~2020-03-11 19:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 14:10 [PATCH -next] x86/mm/pat: mark an intentional data race Qian Cai
2020-03-06 14:16 ` Qian Cai
2020-03-11 16:17 ` Borislav Petkov
2020-03-11 19:04   ` Paul E. McKenney

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