Hi Marco,
On Wed, Oct 16, 2019 at 10:39:58AM +0200, Marco Elver wrote:
> This adds KCSAN instrumentation to atomic-instrumented.h.
>
> Signed-off-by: Marco Elver <el...@google.com>
> ---
> include/asm-generic/atomic-instrumented.h | 192 +++++++++++++++++++++-
> scripts/atomic/gen-atomic-instrumented.sh | 9 +-
> 2 files changed, 199 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/atomic-instrumented.h
> b/include/asm-generic/atomic-instrumented.h
> index e8730c6b9fe2..9e487febc610 100644
> --- a/include/asm-generic/atomic-instrumented.h
> +++ b/include/asm-generic/atomic-instrumented.h
> @@ -19,11 +19,13 @@
>
> #include <linux/build_bug.h>
> #include <linux/kasan-checks.h>
> +#include <linux/kcsan-checks.h>
>
> static inline int
> atomic_read(const atomic_t *v)
> {
> kasan_check_read(v, sizeof(*v));
> + kcsan_check_atomic(v, sizeof(*v), false);
For legibility and consistency with kasan, it would be nicer to avoid
the bool argument here and have kcsan_check_atomic_{read,write}()
helpers...
> diff --git a/scripts/atomic/gen-atomic-instrumented.sh
> b/scripts/atomic/gen-atomic-instrumented.sh
> index e09812372b17..c0553743a6f4 100755
> --- a/scripts/atomic/gen-atomic-instrumented.sh
> +++ b/scripts/atomic/gen-atomic-instrumented.sh
> @@ -12,15 +12,20 @@ gen_param_check()
> local type="${arg%%:*}"
> local name="$(gen_param_name "${arg}")"
> local rw="write"
> + local is_write="true"
>
> case "${type#c}" in
> i) return;;
> esac
>
> # We don't write to constant parameters
> - [ ${type#c} != ${type} ] && rw="read"
> + if [ ${type#c} != ${type} ]; then
> + rw="read"
> + is_write="false"
> + fi
>
> printf "\tkasan_check_${rw}(${name}, sizeof(*${name}));\n"
> + printf "\tkcsan_check_atomic(${name}, sizeof(*${name}), ${is_write});\n"
... which would also simplify this.
Though as below, we might want to wrap both in a helper local to
atomic-instrumented.h.
> }
>
> #gen_param_check(arg...)
> @@ -108,6 +113,7 @@ cat <<EOF
> ({ \\
> typeof(ptr) __ai_ptr = (ptr); \\
> kasan_check_write(__ai_ptr, ${mult}sizeof(*__ai_ptr)); \\
> + kcsan_check_atomic(__ai_ptr, ${mult}sizeof(*__ai_ptr), true); \\
> arch_${xchg}(__ai_ptr, __VA_ARGS__); \\
> })
> EOF
> @@ -148,6 +154,7 @@ cat << EOF
>
> #include <linux/build_bug.h>
> #include <linux/kasan-checks.h>
> +#include <linux/kcsan-checks.h>
We could add the following to this preamble:
static inline void __atomic_check_read(const volatile void *v, size_t size)
{
kasan_check_read(v, sizeof(*v));
kcsan_check_atomic(v, sizeof(*v), false);
}
static inline void __atomic_check_write(const volatile void *v, size_t size)
{
kasan_check_write(v, sizeof(*v));
kcsan_check_atomic(v, sizeof(*v), true);
}
... and only have the one call in each atomic wrapper.
Otherwise, this looks good to me.
Thanks,
Mark.
