All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vincent Donnefort <vdonnefort@google.com>
To: Nathan Chancellor <nathan@kernel.org>
Cc: maz@kernel.org, rostedt@goodmis.org, arnd@arndb.de,
	linux-trace-kernel@vger.kernel.org, kvmarm@lists.linux.dev,
	kernel-team@android.com
Subject: Re: [PATCH] tracing: Generate undef symbols allowlist for simple_ring_buffer
Date: Fri, 13 Mar 2026 10:23:34 +0000	[thread overview]
Message-ID: <abPlpjwY_fT81B-E@google.com> (raw)
In-Reply-To: <20260312235153.GA1147071@ax162>

On Thu, Mar 12, 2026 at 04:51:53PM -0700, Nathan Chancellor wrote:
> Hi Vincent,
> 
> On Thu, Mar 12, 2026 at 06:20:10PM +0000, Vincent Donnefort wrote:
> > Compiler and tooling-generated symbols are difficult to maintain
> > across all supported architectures. Make the allowlist more robust by
> > replacing the harcoded list with a mechanism that automatically detects
> > these symbols.
> > 
> > This mechanism generates a C function designed to trigger common
> > compiler-inserted symbols.
> 
> This certainly seems more robust.
> 
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > 
> > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> > index beb15936829d..3b427b76434a 100644
> > --- a/kernel/trace/Makefile
> > +++ b/kernel/trace/Makefile
> > @@ -136,17 +136,37 @@ obj-$(CONFIG_TRACE_REMOTE_TEST) += remote_test.o
> >  # simple_ring_buffer is used by the pKVM hypervisor which does not have access
> >  # to all kernel symbols. Fail the build if forbidden symbols are found.
> >  #
> > -UNDEFINED_ALLOWLIST := memset alt_cb_patch_nops __x86 __ubsan __asan __kasan __gcov __aeabi_unwind
> > -UNDEFINED_ALLOWLIST += __stack_chk_fail stackleak_track_stack __ref_stack __sanitizer llvm_gcda llvm_gcov
> > -UNDEFINED_ALLOWLIST += .TOC\. __clear_pages_unrolled __memmove copy_page warn_slowpath_fmt
> > -UNDEFINED_ALLOWLIST += ftrace_likely_update __hwasan_load __hwasan_store __hwasan_tag_memory
> > -UNDEFINED_ALLOWLIST += warn_bogus_irq_restore __stack_chk_guard
> > -UNDEFINED_ALLOWLIST := $(addprefix -e , $(UNDEFINED_ALLOWLIST))
> > +# undefsyms_base generates a set of compiler and tooling-generated symbols that can
> > +# safely be ignored for simple_ring_buffer.
> > +#
> > +$(obj)/undefsyms_base.c: FORCE
> > +	$(Q)echo '#include <asm/page.h>' > $@
> > +	$(Q)echo '#include <asm/local.h>' >> $@
> > +	$(Q)echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);' >> $@
> > +	$(Q)echo 'void undefsyms_base(int n);' >> $@
> > +	$(Q)echo 'void undefsyms_base(int n) {' >> $@
> > +	$(Q)echo '	char buffer[256] = { 0 };' >> $@
> > +	$(Q)echo '	u32 u = 0;' >> $@
> > +	$(Q)echo '	memset((char * volatile)page, 8, PAGE_SIZE);' >> $@
> > +	$(Q)echo '	memset((char * volatile)buffer, 8, sizeof(buffer));' >> $@
> > +	$(Q)echo '	cmpxchg((u32 * volatile)&u, 0, 8);' >> $@
> > +	$(Q)echo '	WARN_ON(n == 0xdeadbeef);' >> $@
> > +	$(Q)echo '}' >> $@
> 
> This should use filechk, otherwise undefsyms_base.c will be regenerated
> every build, resulting in undefsyms_base.o being rebuilt every time.
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 mrproper allmodconfig kernel/trace/
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 V=2 kernel/trace/
>   ...
>     CC      kernel/trace/undefsyms_base.o - due to: kernel/trace/undefsyms_base.c
>     NM      kernel/trace/simple_ring_buffer.o - due to target missing
> 
> filechk_undefsyms_base = {                                              \
> 	echo '$(pound)include <asm/page.h>';                            \
> 	echo '$(pound)include <asm/local.h>';                           \
> 	echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);';       \
> 	echo 'void undefsyms_base(int n);';                             \
> 	echo 'void undefsyms_base(int n) {';                            \
> 	echo '	char buffer[256] = { 0 };';                             \
> 	echo '	u32 u = 0;';                                            \
> 	echo '	memset((char * volatile)page, 8, PAGE_SIZE);';          \
> 	echo '	memset((char * volatile)buffer, 8, sizeof(buffer));';   \
> 	echo '	cmpxchg((u32 * volatile)&u, 0, 8);';                    \
> 	echo '	WARN_ON(n == 0xdeadbeef);';                             \
> 	echo '}';                                                       \
> 	}
> 
> $(obj)/undefsyms_base.c: FORCE
> 	$(call filechk,undefsyms_base)
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 mrproper allmodconfig kernel/trace/
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 V=2 kernel/trace/
>     GEN     Makefile - due to target is PHONY
>     DESCEND objtool
>     CALL    scripts/checksyscalls.sh - due to target is PHONY
>     INSTALL libsubcmd_headers
>     NM      kernel/trace/simple_ring_buffer.o - due to target missing
> 
> > +clean-files += undefsyms_base.c
> > +targets += undefsyms_base.c
> 
> I don't think this targets addition is necessary.
> 
> > +$(obj)/undefsyms_base.o: $(obj)/undefsyms_base.c
> > +
> > +extra-y += undefsyms_base.o
> 
> I think this should be
> 
>   targets += undefsyms_base.o
> 
> as extra-y is deprecated per Documentation/kbuild/makefiles.rst.
> 
> > +UNDEFINED_ALLOWLIST = __asan __gcov __kasan __kcsan __hwasan __sanitizer __tsan __ubsan __x86_indirect_thunk \
> > +		       $(shell $(NM) -u $(obj)/undefsyms_base.o 2>/dev/null | awk '{print $$2}')
> 
> With an allmodconfig + ThinLTO build, I still see:
> 
>   $ cat allmod.config
>   CONFIG_GCOV_KERNEL=n
>   CONFIG_KASAN=n
>   CONFIG_LTO_CLANG_THIN=y
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 KCONFIG_ALLCONFIG=1 LLVM=1 mrproper allmodconfig kernel/trace/
>   Unexpected symbols in kernel/trace/simple_ring_buffer.o:
>                    U __fortify_panic
>                    U __write_overflow_field
>                    U simple_ring_buffer_commit
>                    U simple_ring_buffer_enable_tracing
>                    U simple_ring_buffer_init
>                    U simple_ring_buffer_reserve
>                    U simple_ring_buffer_reset
>                    U simple_ring_buffer_swap_reader_page
>                    U simple_ring_buffer_unload
> 
> Something like:
> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index 48c415a0c7e4..0f9a6ce9abd9 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -142,13 +142,15 @@ obj-$(CONFIG_TRACE_REMOTE_TEST) += remote_test.o
>  filechk_undefsyms_base = {                                              \
>  	echo '$(pound)include <asm/page.h>';                            \
>  	echo '$(pound)include <asm/local.h>';                           \
> +	echo '$(pound)include <linux/string.h>';                        \
>  	echo 'static char page[PAGE_SIZE] __aligned(PAGE_SIZE);';       \
> -	echo 'void undefsyms_base(int n);';                             \
> -	echo 'void undefsyms_base(int n) {';                            \
> +	echo 'void undefsyms_base(int n, void *ptr);';                  \
> +	echo 'void undefsyms_base(int n, void *ptr) {';                 \
>  	echo '	char buffer[256] = { 0 };';                             \
>  	echo '	u32 u = 0;';                                            \
>  	echo '	memset((char * volatile)page, 8, PAGE_SIZE);';          \
>  	echo '	memset((char * volatile)buffer, 8, sizeof(buffer));';   \
> +	echo '	memcpy((void* volatile)ptr, buffer, sizeof(buffer));';  \
>  	echo '	cmpxchg((u32 * volatile)&u, 0, 8);';                    \
>  	echo '	WARN_ON(n == 0xdeadbeef);';                             \
>  	echo '}';                                                       \
> --
> 
> cures the first two. The simple_ring_buffer symbols are very odd...
> 
>   $ llvm-nm kernel/trace/simple_ring_buffer.o | grep simple_ring_buffer
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_commit_845
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_enable_tracing_849
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_init_847
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_reserve_841
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_reset_846
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_swap_reader_page_837
>   ---------------- d __UNIQUE_ID_addressable_simple_ring_buffer_unload_848
>   ---------------- t __export_symbol_simple_ring_buffer_commit
>   ---------------- t __export_symbol_simple_ring_buffer_enable_tracing
>   ---------------- t __export_symbol_simple_ring_buffer_init
>   ---------------- t __export_symbol_simple_ring_buffer_reserve
>   ---------------- t __export_symbol_simple_ring_buffer_reset
>   ---------------- t __export_symbol_simple_ring_buffer_swap_reader_page
>   ---------------- t __export_symbol_simple_ring_buffer_unload
>   ---------------- T simple_ring_buffer_commit
>                    U simple_ring_buffer_commit
>   ---------------- T simple_ring_buffer_enable_tracing
>                    U simple_ring_buffer_enable_tracing
>                    U simple_ring_buffer_init
>   ---------------- T simple_ring_buffer_init
>   ---------------- T simple_ring_buffer_init_mm
>   ---------------- T simple_ring_buffer_reserve
>                    U simple_ring_buffer_reserve
>   ---------------- T simple_ring_buffer_reset
>                    U simple_ring_buffer_reset
>                    U simple_ring_buffer_swap_reader_page
>   ---------------- T simple_ring_buffer_swap_reader_page
>                    U simple_ring_buffer_unload
>   ---------------- T simple_ring_buffer_unload
>   ---------------- T simple_ring_buffer_unload_mm
> 
> This is LLVM IR bitcode at this stage, which could be messing things up.
> 
>   $ file kernel/trace/simple_ring_buffer.o
>   kernel/trace/simple_ring_buffer.o: LLVM IR bitcode
> 
> Maybe not worth thinking about too much and just adding it to the
> allowlist manually?

That looks good, thanks for having a look. I'll spin a v2 with your comments.

> 
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index 0f9a6ce9abd9..cb1ec50a8386 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -166,6 +166,7 @@ $(obj)/undefsyms_base.o: $(obj)/undefsyms_base.c
>  targets += undefsyms_base.o
>  
>  UNDEFINED_ALLOWLIST = __asan __gcov __kasan __kcsan __hwasan __sanitizer __tsan __ubsan __x86_indirect_thunk \
> +		       simple_ring_buffer \
>  		       $(shell $(NM) -u $(obj)/undefsyms_base.o 2>/dev/null | awk '{print $$2}')
>  
>  quiet_cmd_check_undefined = NM      $<
> --
> 
> Cheers,
> Nathan

      reply	other threads:[~2026-03-13 10:23 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-12 18:20 [PATCH] tracing: Generate undef symbols allowlist for simple_ring_buffer Vincent Donnefort
2026-03-12 18:43 ` Steven Rostedt
2026-03-12 23:51 ` Nathan Chancellor
2026-03-13 10:23   ` Vincent Donnefort [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=abPlpjwY_fT81B-E@google.com \
    --to=vdonnefort@google.com \
    --cc=arnd@arndb.de \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=nathan@kernel.org \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.