[PATCH 1/2] kcsan: xtensa: Add atomic builtin stubs for 32-bit systems

[Rohan McLure]

> On 16 Feb 2023, at 7:09 pm, Marco Elver <elver at google.com> wrote:
> 
> On Thu, Feb 16, 2023 at 07:12AM +0000, Christophe Leroy wrote:
>> 
>> 
>> Le 16/02/2023 à 06:09, Rohan McLure a écrit :
>>> KCSAN instruments calls to atomic builtins, and will in turn call these
>>> builtins itself. As such, architectures supporting KCSAN must have
>>> compiler support for these atomic primitives.
>>> 
>>> Since 32-bit systems are unlikely to have 64-bit compiler builtins,
>>> provide a stub for each missing builtin, and use BUG() to assert
>>> unreachability.
>>> 
>>> In commit 725aea873261 ("xtensa: enable KCSAN"), xtensa implements these
>>> locally. Move these definitions to be accessible to all 32-bit
>>> architectures that do not provide the necessary builtins, with opt in
>>> for PowerPC and xtensa.
>>> 
>>> Signed-off-by: Rohan McLure <rmclure at linux.ibm.com>
>>> Reviewed-by: Max Filippov <jcmvbkbc at gmail.com>
>> 
>> This series should also be addressed to KCSAN Maintainers, shouldn't it ?
>> 
>> KCSAN
>> M: Marco Elver <elver at google.com>
>> R: Dmitry Vyukov <dvyukov at google.com>
>> L: kasan-dev at googlegroups.com
>> S: Maintained
>> F: Documentation/dev-tools/kcsan.rst
>> F: include/linux/kcsan*.h
>> F: kernel/kcsan/
>> F: lib/Kconfig.kcsan
>> F: scripts/Makefile.kcsan
>> 
>> 
>>> ---
>>> Previously issued as a part of a patch series adding KCSAN support to
>>> 64-bit.
>>> Link: https://lore.kernel.org/linuxppc-dev/167646486000.1421441.10070059569986228558.b4-ty@ellerman.id.au/T/#t
>>> v1: Remove __has_builtin check, as gcc is not obligated to inline
>>> builtins detected using this check, but instead is permitted to supply
>>> them in libatomic:
>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108734
>>> Instead, opt-in PPC32 and xtensa.
>>> ---
>>>  arch/xtensa/lib/Makefile                              | 1 -
>>>  kernel/kcsan/Makefile                                 | 2 ++
>>>  arch/xtensa/lib/kcsan-stubs.c => kernel/kcsan/stubs.c | 0
>>>  3 files changed, 2 insertions(+), 1 deletion(-)
>>>  rename arch/xtensa/lib/kcsan-stubs.c => kernel/kcsan/stubs.c (100%)
>>> 
>>> diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
>>> index 7ecef0519a27..d69356dc97df 100644
>>> --- a/arch/xtensa/lib/Makefile
>>> +++ b/arch/xtensa/lib/Makefile
>>> @@ -8,5 +8,4 @@ lib-y += memcopy.o memset.o checksum.o \
>>>      divsi3.o udivsi3.o modsi3.o umodsi3.o mulsi3.o umulsidi3.o \
>>>      usercopy.o strncpy_user.o strnlen_user.o
>>>  lib-$(CONFIG_PCI) += pci-auto.o
>>> -lib-$(CONFIG_KCSAN) += kcsan-stubs.o
>>>  KCSAN_SANITIZE_kcsan-stubs.o := n
>>> diff --git a/kernel/kcsan/Makefile b/kernel/kcsan/Makefile
>>> index 8cf70f068d92..86dd713d8855 100644
>>> --- a/kernel/kcsan/Makefile
>>> +++ b/kernel/kcsan/Makefile
>>> @@ -12,6 +12,8 @@ CFLAGS_core.o := $(call cc-option,-fno-conserve-stack) \
>>>   -fno-stack-protector -DDISABLE_BRANCH_PROFILING
>>> 
>>>  obj-y := core.o debugfs.o report.o
>>> +obj-$(CONFIG_PPC32) += stubs.o
>>> +obj-$(CONFIG_XTENSA) += stubs.o
>> 
>> Not sure it is acceptable to do it that way.
>> 
>> There should likely be something like a CONFIG_ARCH_WANTS_KCSAN_STUBS in 
>> KCSAN's Kconfig then PPC32 and XTENSA should select it.
> 
> The longer I think about it, since these stubs all BUG() anyway, perhaps
> we ought to just avoid them altogether. If you delete all the stubs from
> ppc and xtensa, but do this:
> 
> | diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> | index 54d077e1a2dc..8169d6dadd0e 100644
> | --- a/kernel/kcsan/core.c
> | +++ b/kernel/kcsan/core.c
> | @@ -1261,7 +1261,9 @@ static __always_inline void kcsan_atomic_builtin_memorder(int memorder)
> |  DEFINE_TSAN_ATOMIC_OPS(8);
> |  DEFINE_TSAN_ATOMIC_OPS(16);
> |  DEFINE_TSAN_ATOMIC_OPS(32);
> | +#ifdef CONFIG_64BIT
> |  DEFINE_TSAN_ATOMIC_OPS(64);
> | +#endif
> |  
> |  void __tsan_atomic_thread_fence(int memorder);
> |  void __tsan_atomic_thread_fence(int memorder)
> 
> Does that work?

This makes much more sense. Rather than assume that kcsan is the only
consumer of __atomic_*_8, and stubbing accordingly, we should just
remove its mention from relevant sub-archs.