Re: [PATCH 7/9] bthread: move asan fiber API into header

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 10.12.24 09:25, Sascha Hauer wrote:
> On Mon, Nov 25, 2024 at 04:35:21PM +0100, Ahmad Fatoum wrote:
>> Using the ASAN fibre API is essential, so the cooperative scheduling we
>> implement on top isn't flagged by AddressSanitizer.
>>
>> To prepare using that code elsewhere later, move it to a common header.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  arch/Kconfig                       |  3 ++
>>  arch/arm/include/asm/setjmp.h      |  2 +
>>  arch/kvx/include/asm/setjmp.h      |  2 +
>>  arch/mips/include/asm/setjmp.h     |  2 +
>>  arch/openrisc/include/asm/setjmp.h |  2 +
>>  arch/powerpc/include/asm/setjmp.h  |  2 +
>>  arch/riscv/include/asm/setjmp.h    |  2 +
>>  arch/sandbox/Kconfig               |  1 +
>>  arch/sandbox/include/asm/setjmp.h  | 42 ++++++++++++++++++++
>>  arch/x86/include/asm/setjmp.h      |  2 +
>>  common/bthread.c                   | 64 ++++++++----------------------
>>  include/asm-generic/setjmp.h       | 27 +++++++++++++
>>  12 files changed, 104 insertions(+), 47 deletions(-)
>>  create mode 100644 include/asm-generic/setjmp.h
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index ad47dbf78d6c..671e6a0e979d 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -18,3 +18,6 @@ config ARCH_HAS_CTRLC
>>  #
>>  config ARCH_DMA_DEFAULT_COHERENT
>>  	bool
>> +
>> +config ARCH_HAS_ASAN_FIBER_API
>> +	bool
>> diff --git a/arch/arm/include/asm/setjmp.h b/arch/arm/include/asm/setjmp.h
>> index 0cee5bfdda60..d28235e56c36 100644
>> --- a/arch/arm/include/asm/setjmp.h
>> +++ b/arch/arm/include/asm/setjmp.h
>> @@ -28,4 +28,6 @@ void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _SETJMP_H_ */
>> diff --git a/arch/kvx/include/asm/setjmp.h b/arch/kvx/include/asm/setjmp.h
>> index 3c23576e6e82..7a09b182f251 100644
>> --- a/arch/kvx/include/asm/setjmp.h
>> +++ b/arch/kvx/include/asm/setjmp.h
>> @@ -12,4 +12,6 @@ int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *sta
>>  int setjmp(jmp_buf jmp) __attribute__((returns_twice));
>>  void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _ASM_KVX_SETJMP_H_ */
>> diff --git a/arch/mips/include/asm/setjmp.h b/arch/mips/include/asm/setjmp.h
>> index 39e01e27dfc0..af25678573e7 100644
>> --- a/arch/mips/include/asm/setjmp.h
>> +++ b/arch/mips/include/asm/setjmp.h
>> @@ -29,4 +29,6 @@ int setjmp(jmp_buf jmp) __attribute__((returns_twice));
>>  void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _MIPS_BITS_SETJMP_H */
>> diff --git a/arch/openrisc/include/asm/setjmp.h b/arch/openrisc/include/asm/setjmp.h
>> index ee73306d189d..d652e8a3cbcc 100644
>> --- a/arch/openrisc/include/asm/setjmp.h
>> +++ b/arch/openrisc/include/asm/setjmp.h
>> @@ -14,4 +14,6 @@ int setjmp(jmp_buf jmp) __attribute__((returns_twice));
>>  void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _OR1K_BITS_SETJMP_H */
>> diff --git a/arch/powerpc/include/asm/setjmp.h b/arch/powerpc/include/asm/setjmp.h
>> index 91bfcdb7f442..1ece16bb7ea7 100644
>> --- a/arch/powerpc/include/asm/setjmp.h
>> +++ b/arch/powerpc/include/asm/setjmp.h
>> @@ -18,4 +18,6 @@ void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _SETJMP_H_ */
>> diff --git a/arch/riscv/include/asm/setjmp.h b/arch/riscv/include/asm/setjmp.h
>> index 468fc4b10aaf..6604ba402bd9 100644
>> --- a/arch/riscv/include/asm/setjmp.h
>> +++ b/arch/riscv/include/asm/setjmp.h
>> @@ -24,4 +24,6 @@ void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif /* _SETJMP_H_ */
>> diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
>> index d4379c4d68db..932bfd15d212 100644
>> --- a/arch/sandbox/Kconfig
>> +++ b/arch/sandbox/Kconfig
>> @@ -43,6 +43,7 @@ config 64BIT
>>  	default CC_IS_64BIT
>>  	select ARCH_DMA_ADDR_T_64BIT
>>  	select PHYS_ADDR_T_64BIT
>> +	select ARCH_HAS_ASAN_FIBER_API if ASAN
>>  
>>  config 32BIT
>>  	def_bool !64BIT
>> diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h
>> index dcc3b4e86712..65a6d5aafa87 100644
>> --- a/arch/sandbox/include/asm/setjmp.h
>> +++ b/arch/sandbox/include/asm/setjmp.h
>> @@ -1,4 +1,9 @@
>>  /* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This file is included both in arch/sandbox/os along with
>> + * system headers and outside with barebox headers, so we avoid
>> + * including any other headers here.
>> + */
>>  
>>  #ifndef __SETJMP_H_
>>  #define __SETJMP_H_
>> @@ -14,4 +19,41 @@ void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn));
>>  
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top);
>>  
>> +#define start_switch_fiber start_switch_fiber
>> +#define finish_switch_fiber finish_switch_fiber
>> +
>> +void __sanitizer_start_switch_fiber(void **fake_stack_save,
>> +                                    const void *bottom, size_t size)
>> +    __attribute__((visibility("default")));
>> +
>> +void __sanitizer_finish_switch_fiber(void *fake_stack_save,
>> +                                     const void **bottom_old, size_t *size_old)
>> +    __attribute__((visibility("default")));
>> +
>> +static inline void finish_switch_fiber(void *fake_stack_save,
>> +                                       void **initial_bottom,
>> +                                       unsigned *initial_stack_size)
>> +{
>> +#ifdef CONFIG_ARCH_HAS_ASAN_FIBER_API
>> +    const void *bottom_old;
>> +    size_t size_old;
>> +
>> +    __sanitizer_finish_switch_fiber(fake_stack_save,
>> +                                    &bottom_old, &size_old);
>> +
>> +    if (initial_bottom && !*initial_bottom) {
>> +        *initial_bottom = (void *)bottom_old;
>> +        *initial_stack_size = size_old;
>> +    }
>> +#endif
>> +}
>> +
>> +static inline void start_switch_fiber(void **fake_stack_save,
>> +                                      const void *bottom, unsigned size)
>> +{
>> +#ifdef CONFIG_ARCH_HAS_ASAN_FIBER_API
>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>> +#endif
>> +}
>> +
>>  #endif
>> diff --git a/arch/x86/include/asm/setjmp.h b/arch/x86/include/asm/setjmp.h
>> index 5af5e624895c..e0f5771b06ba 100644
>> --- a/arch/x86/include/asm/setjmp.h
>> +++ b/arch/x86/include/asm/setjmp.h
>> @@ -41,4 +41,6 @@ void longjmp(jmp_buf jmp, int ret) __attribute__((noreturn)) __sjlj_attr;
>>  
>>  int initjmp(jmp_buf jmp, void __attribute__((noreturn)) (*func)(void), void *stack_top) __sjlj_attr;
>>  
>> +#include <asm-generic/setjmp.h>
>> +
>>  #endif
>> diff --git a/common/bthread.c b/common/bthread.c
>> index d40c0b0f9e3a..943f4c22b346 100644
>> --- a/common/bthread.c
>> +++ b/common/bthread.c
>> @@ -15,10 +15,6 @@
>>  #include <asm/setjmp.h>
>>  #include <linux/overflow.h>
>>  
>> -#if defined CONFIG_ASAN && !defined CONFIG_32BIT
>> -#define HAVE_FIBER_SANITIZER
>> -#endif
>> -
>>  static struct bthread {
>>  	void (*threadfn)(void *);
>>  	void *data;
>> @@ -27,7 +23,7 @@ static struct bthread {
>>  	void *stack;
>>  	u32 stack_size;
>>  	struct list_head list;
>> -#ifdef HAVE_FIBER_SANITIZER
>> +#ifdef CONFIG_ARCH_HAS_ASAN_FIBER_API
>>  	void *fake_stack_save;
>>  #endif
> 
> This breaks compilation of multi_v8_defconfig. Before this change usage
> of fake_stack_save depended on #ifdef HAVE_FIBER_SANITIZER, ...
> 
>>  	u8 awake :1;
>> @@ -45,12 +41,22 @@ struct bthread *current = &main_thread;
>>  /*
>>   * When using ASAN, it needs to be told when we switch stacks.
>>   */
>> -static void start_switch_fiber(struct bthread *, bool terminate_old);
>> -static void finish_switch_fiber(struct bthread *);
>> +static void bthread_finish_switch_fiber(struct bthread *bthread)
>> +{
>> +	finish_switch_fiber(bthread->fake_stack_save,
>> +			    &main_thread.stack, &main_thread.stack_size);
>> +}
> 
> ... but now it's used unconditionally.
> 
> For now I'll just drop the ifdef around void *fake_stack_save. I don't
> know if that's the correct solution, but maybe it doesn't matter as
> fake_stack_save is removed in a later commit anyway.

Yes, it doesn't matter.

Thanks,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |