s390: Always declare __mem functions

PATCH In review Marco Elver 2 participants 7 messages Sep 9, 2022

Subsystems (4)

Stable branch s390 objtool KCSAN

lore

Share & tools

lore

Marco Elver

author maintainer

2022-09-09 07:38 UTC #0 lore raw

PATCH v2 1/3 s390: Always declare __mem functions

Like other architectures, always declare __mem*() functions if the architecture defines __HAVE_ARCH_MEM*.

For example, this is required by sanitizer runtimes to unambiguously refer to the arch versions of the mem-functions, and the compiler not attempting any "optimizations" such as replacing the calls with builtins (which may later be inlined etc.).

Signed-off-by Marco Elver <[email protected]>

---
v2:
* New patch.

---
 arch/s390/include/asm/string.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

arch/s390/include/asm/string.h +3 -4

index 3fae93ddb322..2c3c48d526b9 100644
@@ -20,8 +20,11 @@
 #define __HAVE_ARCH_MEMSET64	/* arch function */
 
 void *memcpy(void *dest, const void *src, size_t n);
+void *__memcpy(void *dest, const void *src, size_t n);
 void *memset(void *s, int c, size_t n);
+void *__memset(void *s, int c, size_t n);
 void *memmove(void *dest, const void *src, size_t n);
+void *__memmove(void *dest, const void *src, size_t n);
 
 #ifndef CONFIG_KASAN
 #define __HAVE_ARCH_MEMCHR	/* inline & arch function */

@@ -55,10 +58,6 @@ char *strstr(const char *s1, const char *s2);
 
 #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
 
-extern void *__memcpy(void *dest, const void *src, size_t n);
-extern void *__memset(void *s, int c, size_t n);
-extern void *__memmove(void *dest, const void *src, size_t n);
-
 /*
  * For files that are not instrumented (e.g. mm/slub.c) we
  * should use not instrumented version of mem* functions.

Signature · 1 line

2.37.2.789.g6183377224-goog

Marco Elver

author maintainer

2022-09-09 07:38 UTC #1 lore raw

PATCH v2 2/3 kcsan: Instrument memcpy/memset/memmove with newer Clang

With Clang version 16+, -fsanitize=thread will turn memcpy/memset/memmove calls in instrumented functions into __tsan_memcpy/__tsan_memset/__tsan_memmove calls respectively.

Add these functions to the core KCSAN runtime, so that we (a) catch data races with mem* functions, and (b) won't run into linker errors with such newer compilers.

Cc [email protected] # v5.10+

Signed-off-by Marco Elver <[email protected]>

---
v2:
* Fix for architectures which do not provide their own memcpy/memset/memmove and instead use the generic versions in lib/string. In this case we'll just alias the __tsan_ variants.

---
 kernel/kcsan/core.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

kernel/kcsan/core.c +39 -0

index fe12dfe254ec..4015f2a3e7f6 100644
@@ -18,6 +18,7 @@
 #include <linux/percpu.h>
 #include <linux/preempt.h>
 #include <linux/sched.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
 
 #include "encoding.h"

@@ -1308,3 +1309,41 @@ noinline void __tsan_atomic_signal_fence(int memorder)
 	}
 }
 EXPORT_SYMBOL(__tsan_atomic_signal_fence);
+
+#ifdef __HAVE_ARCH_MEMSET
+void *__tsan_memset(void *s, int c, size_t count);
+noinline void *__tsan_memset(void *s, int c, size_t count)
+{
+	check_access(s, count, KCSAN_ACCESS_WRITE, _RET_IP_);
+	return __memset(s, c, count);
+}
+#else
+void *__tsan_memset(void *s, int c, size_t count) __alias(memset);
+#endif
+EXPORT_SYMBOL(__tsan_memset);
+
+#ifdef __HAVE_ARCH_MEMMOVE
+void *__tsan_memmove(void *dst, const void *src, size_t len);
+noinline void *__tsan_memmove(void *dst, const void *src, size_t len)
+{
+	check_access(dst, len, KCSAN_ACCESS_WRITE, _RET_IP_);
+	check_access(src, len, 0, _RET_IP_);
+	return __memmove(dst, src, len);
+}
+#else
+void *__tsan_memmove(void *dst, const void *src, size_t len) __alias(memmove);
+#endif
+EXPORT_SYMBOL(__tsan_memmove);
+
+#ifdef __HAVE_ARCH_MEMCPY
+void *__tsan_memcpy(void *dst, const void *src, size_t len);
+noinline void *__tsan_memcpy(void *dst, const void *src, size_t len)
+{
+	check_access(dst, len, KCSAN_ACCESS_WRITE, _RET_IP_);
+	check_access(src, len, 0, _RET_IP_);
+	return __memcpy(dst, src, len);
+}
+#else
+void *__tsan_memcpy(void *dst, const void *src, size_t len) __alias(memcpy);
+#endif
+EXPORT_SYMBOL(__tsan_memcpy);

Signature · 1 line

2.37.2.789.g6183377224-goog

Dmitry Vyukov

reviewer

2022-09-09 08:38 UTC #2 lore raw

Re: [PATCH v2 2/3] kcsan: Instrument memcpy/memset/memmove with newer Clang

Quoted 33 lines · Marco Elver

With Clang version 16+, -fsanitize=thread will turn memcpy/memset/memmove calls in instrumented functions into __tsan_memcpy/__tsan_memset/__tsan_memmove calls respectively.
Add these functions to the core KCSAN runtime, so that we (a) catch data races with mem* functions, and (b) won't run into linker errors with such newer compilers.
Cc [email protected] # v5.10+
Signed-off-by Marco Elver <[email protected]>
---
v2:
* Fix for architectures which do not provide their own memcpy/memset/memmove and instead use the generic versions in lib/string. In this case we'll just alias the __tsan_ variants.
---
 kernel/kcsan/core.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
kernel/kcsan/core.c +1 -0
index fe12dfe254ec..4015f2a3e7f6 100644
@@ -18,6 +18,7 @@
 #include <linux/percpu.h>
 #include <linux/preempt.h>
 #include <linux/sched.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
 
 #include "encoding.h"

kernel/kcsan/core.c noinline void __tsan_atomic_signal_fence(int memorder) · line 1317

Quoted 9 lines

kernel/kcsan/core.c +6 -0

@@ -1308,3 +1309,41 @@ noinline void __tsan_atomic_signal_fence(int memorder)
        }
 }
 EXPORT_SYMBOL(__tsan_atomic_signal_fence);
+
+#ifdef __HAVE_ARCH_MEMSET
+void *__tsan_memset(void *s, int c, size_t count);
+noinline void *__tsan_memset(void *s, int c, size_t count)
+{
+       check_access(s, count, KCSAN_ACCESS_WRITE, _RET_IP_);

These can use large sizes, does it make sense to truncate it to MAX_ENCODABLE_SIZE?

Quoted 35 lines

kernel/kcsan/core.c +32 -1

+       return __memset(s, c, count);
+}
+#else
+void *__tsan_memset(void *s, int c, size_t count) __alias(memset);
+#endif
+EXPORT_SYMBOL(__tsan_memset);
+
+#ifdef __HAVE_ARCH_MEMMOVE
+void *__tsan_memmove(void *dst, const void *src, size_t len);
+noinline void *__tsan_memmove(void *dst, const void *src, size_t len)
+{
+       check_access(dst, len, KCSAN_ACCESS_WRITE, _RET_IP_);
+       check_access(src, len, 0, _RET_IP_);
+       return __memmove(dst, src, len);
+}
+#else
+void *__tsan_memmove(void *dst, const void *src, size_t len) __alias(memmove);
+#endif
+EXPORT_SYMBOL(__tsan_memmove);
+
+#ifdef __HAVE_ARCH_MEMCPY
+void *__tsan_memcpy(void *dst, const void *src, size_t len);
+noinline void *__tsan_memcpy(void *dst, const void *src, size_t len)
+{
+       check_access(dst, len, KCSAN_ACCESS_WRITE, _RET_IP_);
+       check_access(src, len, 0, _RET_IP_);
+       return __memcpy(dst, src, len);
+}
+#else
+void *__tsan_memcpy(void *dst, const void *src, size_t len) __alias(memcpy);
+#endif
+EXPORT_SYMBOL(__tsan_memcpy);
--

2.37.2.789.g6183377224-goog

Marco Elver

author maintainer

2022-09-09 08:44 UTC #3 lore raw

Re: [PATCH v2 2/3] kcsan: Instrument memcpy/memset/memmove with newer Clang

Quoted 33 lines · Dmitry Vyukov

Quoted 31 lines · Marco Elver
With Clang version 16+, -fsanitize=thread will turn memcpy/memset/memmove calls in instrumented functions into __tsan_memcpy/__tsan_memset/__tsan_memmove calls respectively.
Add these functions to the core KCSAN runtime, so that we (a) catch data races with mem* functions, and (b) won't run into linker errors with such newer compilers.
Cc [email protected] # v5.10+
Signed-off-by Marco Elver <[email protected]>
---
v2:
* Fix for architectures which do not provide their own memcpy/memset/memmove and instead use the generic versions in lib/string. In this case we'll just alias the __tsan_ variants.
---
 kernel/kcsan/core.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
kernel/kcsan/core.c +1 -0
index fe12dfe254ec..4015f2a3e7f6 100644
@@ -18,6 +18,7 @@
 #include <linux/percpu.h>
 #include <linux/preempt.h>
 #include <linux/sched.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
 

Quoted 14 lines

kernel/kcsan/core.c noinline void __tsan_atomic_signal_fence(int memorder) · line 1317
Quoted 11 lines
kernel/kcsan/core.c +6 -0
 #include "encoding.h"
@@ -1308,3 +1309,41 @@ noinline void __tsan_atomic_signal_fence(int memorder)
        }
 }
 EXPORT_SYMBOL(__tsan_atomic_signal_fence);
+
+#ifdef __HAVE_ARCH_MEMSET
+void *__tsan_memset(void *s, int c, size_t count);
+noinline void *__tsan_memset(void *s, int c, size_t count)
+{
+       check_access(s, count, KCSAN_ACCESS_WRITE, _RET_IP_);
These can use large sizes, does it make sense to truncate it to MAX_ENCODABLE_SIZE?

Hmm, good point - that way it can still set up watchpoints on them. I'll do a v3.

Marco Elver

author maintainer

2022-09-09 07:38 UTC #4 lore raw

PATCH v2 3/3 objtool, kcsan: Add volatile read/write instrumentation to whitelist

In reply to Marco Elver

Adds KCSAN's volatile instrumentation to objtool's uaccess whitelist.

Recent kernel change have shown that this was missing from the uaccess whitelist (since the first upstreamed version of KCSAN):

mm/gup.o: warning: objtool: fault_in_readable+0x101: call to __tsan_volatile_write1() with UACCESS enabled

Fixes 75d75b7a4d54 ("kcsan: Support distinguishing volatile accesses")

Signed-off-by Marco Elver <[email protected]>

---
v2:
* Fix commit message.

---
 tools/objtool/check.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

tools/objtool/check.c +10 -0

index e55fdf952a3a..67afdce3421f 100644
@@ -999,6 +999,16 @@ static const char *uaccess_safe_builtin[] = {
 	"__tsan_read_write4",
 	"__tsan_read_write8",
 	"__tsan_read_write16",
+	"__tsan_volatile_read1",
+	"__tsan_volatile_read2",
+	"__tsan_volatile_read4",
+	"__tsan_volatile_read8",
+	"__tsan_volatile_read16",
+	"__tsan_volatile_write1",
+	"__tsan_volatile_write2",
+	"__tsan_volatile_write4",
+	"__tsan_volatile_write8",
+	"__tsan_volatile_write16",
 	"__tsan_atomic8_load",
 	"__tsan_atomic16_load",
 	"__tsan_atomic32_load",

Signature · 1 line

2.37.2.789.g6183377224-goog

Dmitry Vyukov

reviewer Reviewed-by on v2 3/3

2022-09-09 08:38 UTC #5 lore raw

Re: [PATCH v2 3/3] objtool, kcsan: Add volatile read/write instrumentation to whitelist

Quoted 10 lines · Marco Elver

Adds KCSAN's volatile instrumentation to objtool's uaccess whitelist.
Recent kernel change have shown that this was missing from the uaccess whitelist (since the first upstreamed version of KCSAN):
mm/gup.o: warning: objtool: fault_in_readable+0x101: call to __tsan_volatile_write1() with UACCESS enabled
Fixes 75d75b7a4d54 ("kcsan: Support distinguishing volatile accesses")
Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

Quoted 31 lines

---
v2:
* Fix commit message.

---
 tools/objtool/check.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

tools/objtool/check.c +10 -1

index e55fdf952a3a..67afdce3421f 100644
@@ -999,6 +999,16 @@ static const char *uaccess_safe_builtin[] = {
        "__tsan_read_write4",
        "__tsan_read_write8",
        "__tsan_read_write16",
+       "__tsan_volatile_read1",
+       "__tsan_volatile_read2",
+       "__tsan_volatile_read4",
+       "__tsan_volatile_read8",
+       "__tsan_volatile_read16",
+       "__tsan_volatile_write1",
+       "__tsan_volatile_write2",
+       "__tsan_volatile_write4",
+       "__tsan_volatile_write8",
+       "__tsan_volatile_write16",
        "__tsan_atomic8_load",
        "__tsan_atomic16_load",
        "__tsan_atomic32_load",
--

2.37.2.789.g6183377224-goog

Dmitry Vyukov

reviewer Acked-by on v2 1/3

2022-09-09 08:36 UTC #6 lore raw

In reply to Marco Elver

Quoted 10 lines · Marco Elver

Like other architectures, always declare __mem*() functions if the architecture defines __HAVE_ARCH_MEM*.
For example, this is required by sanitizer runtimes to unambiguously refer to the arch versions of the mem-functions, and the compiler not attempting any "optimizations" such as replacing the calls with builtins (which may later be inlined etc.).
Signed-off-by Marco Elver <[email protected]>

Acked-by Dmitry Vyukov <[email protected]>

Quoted 37 lines

---
v2:
* New patch.

---
 arch/s390/include/asm/string.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

arch/s390/include/asm/string.h +3 -5

index 3fae93ddb322..2c3c48d526b9 100644
@@ -20,8 +20,11 @@
 #define __HAVE_ARCH_MEMSET64   /* arch function */
 
 void *memcpy(void *dest, const void *src, size_t n);
+void *__memcpy(void *dest, const void *src, size_t n);
 void *memset(void *s, int c, size_t n);
+void *__memset(void *s, int c, size_t n);
 void *memmove(void *dest, const void *src, size_t n);
+void *__memmove(void *dest, const void *src, size_t n);
 
 #ifndef CONFIG_KASAN
 #define __HAVE_ARCH_MEMCHR     /* inline & arch function */

@@ -55,10 +58,6 @@ char *strstr(const char *s1, const char *s2);
 
 #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
 
-extern void *__memcpy(void *dest, const void *src, size_t n);
-extern void *__memset(void *s, int c, size_t n);
-extern void *__memmove(void *dest, const void *src, size_t n);
-
 /*
  * For files that are not instrumented (e.g. mm/slub.c) we
  * should use not instrumented version of mem* functions.
--

2.37.2.789.g6183377224-goog