Re: [PINGv2][PATCH] Ignore alignment by option

[Dmitry Vyukov <dvyukov at google dot com>]

+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.zakirov@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.zakirov@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.zakirov@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 0000000..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);
> +  *(volatile unsigned int *)(0x12);
> +  *(volatile unsigned long long *)(0x12);
> +  *new_p = *old_p;
> +}
> +
> +/* { dg-final { scan-tree-dump-times ">> 3" 11 "sanopt" } } */
> +/* { dg-final { scan-tree-dump-times "& 7" 11 "sanopt" } } */
> +/* { dg-final { cleanup-tree-dump "sanopt" } } */
>