Re: [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline

[Marco Elver]

On Fri, 29 May 2020 at 19:08, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
[...]
>
> Doesn't this mean we can do the below?

If nobody complains about the lack of __no_kcsan_or_inline, let's do
it. See comments below.

> ---
>  Documentation/dev-tools/kcsan.rst |  6 ------
>  arch/x86/include/asm/bitops.h     |  6 +-----
>  include/linux/compiler_types.h    | 14 ++++----------
>  kernel/kcsan/kcsan-test.c         |  4 ++--
>  4 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
> index ce4bbd918648..b38379f06194 100644
> --- a/Documentation/dev-tools/kcsan.rst
> +++ b/Documentation/dev-tools/kcsan.rst
> @@ -114,12 +114,6 @@ functions, compilation units, or entire subsystems.  For static blacklisting,
>    To dynamically limit for which functions to generate reports, see the
>    `DebugFS interface`_ blacklist/whitelist feature.
>
> -  For ``__always_inline`` functions, replace ``__always_inline`` with
> -  ``__no_kcsan_or_inline`` (which implies ``__always_inline``)::
> -
> -    static __no_kcsan_or_inline void foo(void) {
> -        ...
> -
>  * To disable data race detection for a particular compilation unit, add to the
>    ``Makefile``::

I suppose, if we say that __no_kcsan_or_inline should just disappear
because '__no_kcsan inline' is now good enough, we can delete it.

I think functions that absolutely must be __always_inline would break
with __no_kcsan_or_inline under KCSAN anyway. So, let's simplify.

> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index 35460fef39b8..0367efdc5b7a 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -201,12 +201,8 @@ arch_test_and_change_bit(long nr, volatile unsigned long *addr)
>         return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
>  }
>
> -static __no_kcsan_or_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
> +static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
>  {
> -       /*
> -        * Because this is a plain access, we need to disable KCSAN here to
> -        * avoid double instrumentation via instrumented bitops.
> -        */

Yes, we should have reverted this eventually.

>         return ((1UL << (nr & (BITS_PER_LONG-1))) &
>                 (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
>  }
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 4e4982d6f3b0..6a2c0f857ac3 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -118,10 +118,6 @@ struct ftrace_likely_data {
>  #define notrace                        __attribute__((__no_instrument_function__))
>  #endif
>
> -/* Section for code which can't be instrumented at all */
> -#define noinstr                                                                \
> -       noinline notrace __attribute((__section__(".noinstr.text")))
> -
>  /*
>   * it doesn't make sense on ARM (currently the only user of __naked)
>   * to trace naked functions because then mcount is called without
> @@ -192,17 +188,15 @@ struct ftrace_likely_data {
>  #endif
>
>  #define __no_kcsan __no_sanitize_thread
> -#ifdef __SANITIZE_THREAD__
> -# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
> -# define __no_sanitize_or_inline __no_kcsan_or_inline

I think we just want to keep __no_sanitize_or_inline, for
READ_ONCE_NOCHECK. Having READ_ONCE_NOCHECK do KCSAN-checking seems
wrong, and I don't know what might break.

> -#else
> -# define __no_kcsan_or_inline __always_inline
> -#endif
>
>  #ifndef __no_sanitize_or_inline
>  #define __no_sanitize_or_inline __always_inline
>  #endif
>
> +/* Section for code which can't be instrumented at all */
> +#define noinstr                                                                \
> +       noinline notrace __attribute((__section__(".noinstr.text"))) __no_kcsan
> +

Will this eventually need __no_sanitize_address?

>  #endif /* __KERNEL__ */
>
>  #endif /* __ASSEMBLY__ */
> diff --git a/kernel/kcsan/kcsan-test.c b/kernel/kcsan/kcsan-test.c
> index a8c11506dd2a..374263ddffe2 100644
> --- a/kernel/kcsan/kcsan-test.c
> +++ b/kernel/kcsan/kcsan-test.c
> @@ -43,7 +43,7 @@ static struct {
>  };
>
>  /* Setup test checking loop. */
> -static __no_kcsan_or_inline void
> +static __no_kcsan inline void
>  begin_test_checks(void (*func1)(void), void (*func2)(void))
>  {
>         kcsan_disable_current();
> @@ -60,7 +60,7 @@ begin_test_checks(void (*func1)(void), void (*func2)(void))
>  }
>
>  /* End test checking loop. */
> -static __no_kcsan_or_inline bool
> +static __no_kcsan inline bool
>  end_test_checks(bool stop)
>  {
>         if (!stop && time_before(jiffies, end_time)) {

Acked -- if you send a patch, do split the test-related change, so
that Paul can apply it to the test which is currently only in -rcu.

Thanks,
-- Marco