Re: [PINGv2][PATCH] Ignore alignment by option
+address-sanitizer
Please don't hurry with it.
Do you have any numbers on how frequent are unaligned accesses in
kernel? Is it worth addressing at this cost?
size_in_bytes = -1 instrumentation is too slow to be the default in kernel.
If we want to pursue this, I propose a different scheme.
Handle 8+ byte accesses as 1/2/4 accesses. No changes to 1/2/4 access handling.
Currently when we allocate, say, 17-byte object we store 0 0 1 into
shadow. An 8-byte access starting at offset 15 won't be detected,
because the corresponding shadow value is 0. Instead we start storing
0 9 1 into shadow. Then the first shadow != 0 check will fail, and the
precise size check will catch the OOB access.
Make this scheme the default for kernel (no additional flags).
This scheme has the following advantages:
- load shadow only once (as opposed to the current size_in_bytes = -1
check that loads shadow twice)
- less code in instrumentation
- accesses to beginning and middle of the object are not slowed down
(shadow still contains 0, so fast-path works); only accesses to the
very last bytes of the object are penalized.
On Thu, Dec 4, 2014 at 3:05 PM, Marat Zakirov m.zaki...@samsung.com wrote:
On 11/27/2014 05:14 PM, Marat Zakirov wrote:
On 11/19/2014 06:01 PM, Marat Zakirov wrote:
Hi all!
Here is the patch which forces ASan to ignore alignment of memory access.
It increases ASan overhead but it's still useful because some programs like
linux kernel often cheat with alignment which may cause false negatives.
--Marat
gcc/ChangeLog:
2014-11-14 Marat Zakirov m.zaki...@samsung.com
* asan.c (asan_expand_check_ifn): Ignore alignment by option.
* doc/invoke.texi: Document.
* params.def (asan-alignment-optimize): New.
* params.h: Likewise.
gcc/testsuite/ChangeLog:
2014-11-14 Marat Zakirov m.zaki...@samsung.com
* c-c++-common/asan/red-align-3.c: New test.
diff --git a/gcc/asan.c b/gcc/asan.c
index 79dede7..4f86088 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2518,6 +2518,12 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter,
bool use_calls)
HOST_WIDE_INT size_in_bytes
= is_scalar_access tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1;
+
+ if (!ASAN_ALIGNMENT_OPTIMIZE size_in_bytes 1)
+{
+ size_in_bytes = -1;
+ align = 1;
+}
if (use_calls)
{
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 13270bc..8f43c06 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10580,6 +10580,12 @@ is greater or equal to this number, use callbacks
instead of inline checks.
E.g. to disable inline code use
@option{--param asan-instrumentation-with-call-threshold=0}.
+@item asan-alignment-optimize
+Enable asan optimization for aligned accesses.
+It is enabled by default when using @option{-fsanitize=address} option.
+To disable optimization for aligned accesses use
+@option{--param asan-alignment-optimize=0}.
+
@item chkp-max-ctor-size
Static constructors generated by Pointer Bounds Checker may become very
large and significantly increase compile time at optimization level
diff --git a/gcc/params.def b/gcc/params.def
index d2d2add..fbccf46 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1114,6 +1114,11 @@ DEFPARAM
(PARAM_ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD,
in function becomes greater or equal to this number,
7000, 0, INT_MAX)
+DEFPARAM (PARAM_ASAN_ALIGNMENT_OPTIMIZE,
+ asan-alignment-optimize,
+ Enable asan optimization for aligned access,
+ 1, 0, 1)
+
DEFPARAM (PARAM_UNINIT_CONTROL_DEP_ATTEMPTS,
uninit-control-dep-attempts,
Maximum number of nested calls to search for control dependencies
diff --git a/gcc/params.h b/gcc/params.h
index 4779e17..e2973d4 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -238,5 +238,7 @@ extern void init_param_values (int *params);
PARAM_VALUE (PARAM_ASAN_USE_AFTER_RETURN)
#define ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD \
PARAM_VALUE (PARAM_ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD)
+#define ASAN_ALIGNMENT_OPTIMIZE \
+ PARAM_VALUE (PARAM_ASAN_ALIGNMENT_OPTIMIZE)
#endif /* ! GCC_PARAMS_H */
diff --git a/gcc/testsuite/c-c++-common/asan/ignore_align.c
b/gcc/testsuite/c-c++-common/asan/ignore_align.c
new file mode 100644
index 000..989958b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/ignore_align.c
@@ -0,0 +1,34 @@
+/* { dg-options -fdump-tree-sanopt --param asan-alignment-optimize=0 } */
+/* { dg-do compile } */
+/* { dg-skip-if { *-*-* } { -flto } { } } */
+
+#ifdef __cplusplus
+extern C {
+#endif
+void *memset (void *, int, __SIZE_TYPE__);
+#ifdef __cplusplus
+}
+#endif
+
+struct dummy {
+ int a;
+ int b;
+ int c;
+ int d;
+};
+
+volatile struct dummy * new_p;
+volatile struct dummy * old_p;
+
+void foo(void)
+{
+ *(volatile char *)(0x12);
+ *(volatile short int *)(0x12);
+
Re: [PINGv2][PATCH] Ignore alignment by option
On 12/04/2014 03:47 PM, Dmitry Vyukov wrote: size_in_bytes = -1 instrumentation is too slow to be the default in kernel. If we want to pursue this, I propose a different scheme. Handle 8+ byte accesses as 1/2/4 accesses. No changes to 1/2/4 access handling. Currently when we allocate, say, 17-byte object we store 0 0 1 into shadow. An 8-byte access starting at offset 15 won't be detected, because the corresponding shadow value is 0. Instead we start storing 0 9 1 into shadow. Then the first shadow != 0 check will fail, and the precise size check will catch the OOB access. Make this scheme the default for kernel (no additional flags). This scheme has the following advantages: - load shadow only once (as opposed to the current size_in_bytes = -1 check that loads shadow twice) - less code in instrumentation - accesses to beginning and middle of the object are not slowed down (shadow still contains 0, so fast-path works); only accesses to the very last bytes of the object are penalized. Makes sense. The scheme actually looks bullet-proof, why Asan team preferred current (fast but imprecise) algorithm? BTW I think we'll want this option in userspace so well so we'll probably need to update libasan. -Y
Re: [PINGv2][PATCH] Ignore alignment by option
On Thu, Dec 4, 2014 at 4:48 PM, Yury Gribov y.gri...@samsung.com wrote: On 12/04/2014 03:47 PM, Dmitry Vyukov wrote: size_in_bytes = -1 instrumentation is too slow to be the default in kernel. If we want to pursue this, I propose a different scheme. Handle 8+ byte accesses as 1/2/4 accesses. No changes to 1/2/4 access handling. Currently when we allocate, say, 17-byte object we store 0 0 1 into shadow. An 8-byte access starting at offset 15 won't be detected, because the corresponding shadow value is 0. Instead we start storing 0 9 1 into shadow. Then the first shadow != 0 check will fail, and the precise size check will catch the OOB access. Make this scheme the default for kernel (no additional flags). This scheme has the following advantages: - load shadow only once (as opposed to the current size_in_bytes = -1 check that loads shadow twice) - less code in instrumentation - accesses to beginning and middle of the object are not slowed down (shadow still contains 0, so fast-path works); only accesses to the very last bytes of the object are penalized. Makes sense. The scheme actually looks bullet-proof, why Asan team preferred current (fast but imprecise) algorithm? BTW I think we'll want this option in userspace so well so we'll probably need to update libasan. We've discussed this scheme, but nobody has shown that it's important enough. It bloats binary (we do have issues with binary sizes) and slows down execution a bit. And if it is non-default mode, then it adds more flags (which is bad) and adds more configurations to test. For this to happen somebody needs to do research on (1) binary size increase, (2) slowdown, (3) number of additional bugs it finds (we can run it over extensive code base that is currently asan-clean). Here is the issue with some notes: https://code.google.com/p/address-sanitizer/issues/detail?id=100
Re: [PINGv2][PATCH] Ignore alignment by option
On 12/04/2014 05:04 PM, Dmitry Vyukov wrote: On Thu, Dec 4, 2014 at 4:48 PM, Yury Gribov y.gri...@samsung.com wrote: On 12/04/2014 03:47 PM, Dmitry Vyukov wrote: size_in_bytes = -1 instrumentation is too slow to be the default in kernel. If we want to pursue this, I propose a different scheme. Handle 8+ byte accesses as 1/2/4 accesses. No changes to 1/2/4 access handling. Currently when we allocate, say, 17-byte object we store 0 0 1 into shadow. An 8-byte access starting at offset 15 won't be detected, because the corresponding shadow value is 0. Instead we start storing 0 9 1 into shadow. Then the first shadow != 0 check will fail, and the precise size check will catch the OOB access. Make this scheme the default for kernel (no additional flags). This scheme has the following advantages: - load shadow only once (as opposed to the current size_in_bytes = -1 check that loads shadow twice) - less code in instrumentation - accesses to beginning and middle of the object are not slowed down (shadow still contains 0, so fast-path works); only accesses to the very last bytes of the object are penalized. Makes sense. The scheme actually looks bullet-proof, why Asan team preferred current (fast but imprecise) algorithm? BTW I think we'll want this option in userspace so well so we'll probably need to update libasan. We've discussed this scheme, but nobody has shown that it's important enough. It bloats binary (we do have issues with binary sizes) and slows down execution a bit. And if it is non-default mode, then it adds more flags (which is bad) and adds more configurations to test. For this to happen somebody needs to do research on (1) binary size increase, (2) slowdown, (3) number of additional bugs it finds (we can run it over extensive code base that is currently asan-clean). Regarding (3) - unless a codebase deliberately uses unaligned accesses (like kernel) this change would be of little use - all unaligned accesses are then bugs and should already be detected and fixed with UBSan. -Y
Re: [PINGv2][PATCH] Ignore alignment by option
On Thu, Dec 4, 2014 at 5:16 PM, Yury Gribov y.gri...@samsung.com wrote: On 12/04/2014 05:04 PM, Dmitry Vyukov wrote: On Thu, Dec 4, 2014 at 4:48 PM, Yury Gribov y.gri...@samsung.com wrote: On 12/04/2014 03:47 PM, Dmitry Vyukov wrote: size_in_bytes = -1 instrumentation is too slow to be the default in kernel. If we want to pursue this, I propose a different scheme. Handle 8+ byte accesses as 1/2/4 accesses. No changes to 1/2/4 access handling. Currently when we allocate, say, 17-byte object we store 0 0 1 into shadow. An 8-byte access starting at offset 15 won't be detected, because the corresponding shadow value is 0. Instead we start storing 0 9 1 into shadow. Then the first shadow != 0 check will fail, and the precise size check will catch the OOB access. Make this scheme the default for kernel (no additional flags). This scheme has the following advantages: - load shadow only once (as opposed to the current size_in_bytes = -1 check that loads shadow twice) - less code in instrumentation - accesses to beginning and middle of the object are not slowed down (shadow still contains 0, so fast-path works); only accesses to the very last bytes of the object are penalized. Makes sense. The scheme actually looks bullet-proof, why Asan team preferred current (fast but imprecise) algorithm? BTW I think we'll want this option in userspace so well so we'll probably need to update libasan. We've discussed this scheme, but nobody has shown that it's important enough. It bloats binary (we do have issues with binary sizes) and slows down execution a bit. And if it is non-default mode, then it adds more flags (which is bad) and adds more configurations to test. For this to happen somebody needs to do research on (1) binary size increase, (2) slowdown, (3) number of additional bugs it finds (we can run it over extensive code base that is currently asan-clean). Regarding (3) - unless a codebase deliberately uses unaligned accesses (like kernel) this change would be of little use - all unaligned accesses are then bugs and should already be detected and fixed with UBSan. You answered your own question about user space :)
Re: [PINGv2][PATCH] Ignore alignment by option
On Thu, Dec 4, 2014 at 8:06 PM, 'Dmitry Vyukov' via address-sanitizer address-saniti...@googlegroups.com wrote: You answered your own question about user space :) Yeah, I hoped someone would rush to overpersuade me... -Y
Re: [PINGv2][PATCH] Ignore alignment by option
On Thu, Dec 04, 2014 at 08:26:24PM +0300, Yuri Gribov wrote: On Thu, Dec 4, 2014 at 8:06 PM, 'Dmitry Vyukov' via address-sanitizer address-saniti...@googlegroups.com wrote: You answered your own question about user space :) Yeah, I hoped someone would rush to overpersuade me... While in C unaligned accesses are UB, I think still many userland programs actually use those, not just the kernel, on the architectures known not to SIGILL on those. Though, of course, say on x86_64/i686, when not vectorized unaligned access works just fine, but when vectorized it does not. Jakub
