| Message ID | 20250320070746.101552-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gendwarfksyms: define __GENKSYMS__ when processing asm-protoypes.h | expand |
On Thu, 20 Mar 2025 at 08:08, Ard Biesheuvel <ardb+git@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > through the compiler to capture the exported symbols. This ensures that > exported symbols such as __ref_stack_chk_guard on x86, which is declared > conditionally, is visible to the tool. > > Otherwise, an error such as the below may be raised, breaking the build > when CONFIG_GENDWARFKSYMS=y > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > Cc: Sami Tolvanen <samitolvanen@google.com> > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Tested-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?extid=06fd1a3613c50d36129e > --- > scripts/Makefile.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 993708d11874..7855cdc4e763 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > getasmexports = \ > { echo "\#include <linux/kernel.h>" ; \ > echo "\#include <linux/string.h>" ; \ > + echo "\#define __GENKSYMS__" ; \ > echo "\#include <asm/asm-prototypes.h>" ; \ > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > -- > 2.49.0.rc1.451.g8f38331e32-goog >
On Thu, Mar 20, 2025 at 4:08 PM Ard Biesheuvel <ardb+git@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > through the compiler to capture the exported symbols. This ensures that > exported symbols such as __ref_stack_chk_guard on x86, which is declared > conditionally, is visible to the tool. Why don't you make it unconditional, then? diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h index 3674006e3974..9fc6f2be663c 100644 --- a/arch/x86/include/asm/asm-prototypes.h +++ b/arch/x86/include/asm/asm-prototypes.h @@ -20,6 +20,4 @@ extern void cmpxchg8b_emu(void); #endif -#if defined(__GENKSYMS__) && defined(CONFIG_STACKPROTECTOR) extern unsigned long __ref_stack_chk_guard; -#endif > > Otherwise, an error such as the below may be raised, breaking the build > when CONFIG_GENDWARFKSYMS=y > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > Cc: Sami Tolvanen <samitolvanen@google.com> > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > scripts/Makefile.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 993708d11874..7855cdc4e763 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > getasmexports = \ > { echo "\#include <linux/kernel.h>" ; \ > echo "\#include <linux/string.h>" ; \ > + echo "\#define __GENKSYMS__" ; \ > echo "\#include <asm/asm-prototypes.h>" ; \ > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > -- > 2.49.0.rc1.451.g8f38331e32-goog >
On Thu, 20 Mar 2025 at 15:40, Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Mar 20, 2025 at 4:08 PM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > > through the compiler to capture the exported symbols. This ensures that > > exported symbols such as __ref_stack_chk_guard on x86, which is declared > > conditionally, is visible to the tool. > > > Why don't you make it unconditional, then? > Because this symbol is being hidden from the compiler. Commit 577c134d311b9b has the details.
Hi Ard, On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > through the compiler to capture the exported symbols. This ensures that > exported symbols such as __ref_stack_chk_guard on x86, which is declared > conditionally, is visible to the tool. > > Otherwise, an error such as the below may be raised, breaking the build > when CONFIG_GENDWARFKSYMS=y > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > Cc: Sami Tolvanen <samitolvanen@google.com> > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > scripts/Makefile.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 993708d11874..7855cdc4e763 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > getasmexports = \ > { echo "\#include <linux/kernel.h>" ; \ > echo "\#include <linux/string.h>" ; \ > + echo "\#define __GENKSYMS__" ; \ > echo "\#include <asm/asm-prototypes.h>" ; \ > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } This works with gendwarfksyms since __GENKSYMS__ is defined after the EXPORT_SYMBOL() definition, but I'm now getting warnings with genksyms: AS arch/x86/lib/clear_page_64.o <stdin>:3:10: warning: "__GENKSYMS__" redefined <command-line>: note: this is the location of the previous definition Sami
On Thu, 20 Mar 2025 at 15:48, Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi Ard, > > On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > > through the compiler to capture the exported symbols. This ensures that > > exported symbols such as __ref_stack_chk_guard on x86, which is declared > > conditionally, is visible to the tool. > > > > Otherwise, an error such as the below may be raised, breaking the build > > when CONFIG_GENDWARFKSYMS=y > > > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > > > Cc: Sami Tolvanen <samitolvanen@google.com> > > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > scripts/Makefile.build | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > index 993708d11874..7855cdc4e763 100644 > > --- a/scripts/Makefile.build > > +++ b/scripts/Makefile.build > > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > > getasmexports = \ > > { echo "\#include <linux/kernel.h>" ; \ > > echo "\#include <linux/string.h>" ; \ > > + echo "\#define __GENKSYMS__" ; \ > > echo "\#include <asm/asm-prototypes.h>" ; \ > > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > This works with gendwarfksyms since __GENKSYMS__ is defined after the > EXPORT_SYMBOL() definition, but I'm now getting warnings with > genksyms: > > AS arch/x86/lib/clear_page_64.o > <stdin>:3:10: warning: "__GENKSYMS__" redefined > <command-line>: note: this is the location of the previous definition > Oops. Do you think the fix below should be sufficient? --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -314,7 +314,7 @@ else cmd_gensymtypes_S = \ $(getasmexports) | \ - $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms) + $(CPP) $(c_flags) -xc - | $(genksyms) endif # CONFIG_GENDWARFKSYMS quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
On Thu, Mar 20, 2025 at 9:09 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 20 Mar 2025 at 15:48, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Hi Ard, > > > > On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > > > through the compiler to capture the exported symbols. This ensures that > > > exported symbols such as __ref_stack_chk_guard on x86, which is declared > > > conditionally, is visible to the tool. > > > > > > Otherwise, an error such as the below may be raised, breaking the build > > > when CONFIG_GENDWARFKSYMS=y > > > > > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > > > > > Cc: Sami Tolvanen <samitolvanen@google.com> > > > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > scripts/Makefile.build | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > > index 993708d11874..7855cdc4e763 100644 > > > --- a/scripts/Makefile.build > > > +++ b/scripts/Makefile.build > > > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > > > getasmexports = \ > > > { echo "\#include <linux/kernel.h>" ; \ > > > echo "\#include <linux/string.h>" ; \ > > > + echo "\#define __GENKSYMS__" ; \ > > > echo "\#include <asm/asm-prototypes.h>" ; \ > > > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > > > This works with gendwarfksyms since __GENKSYMS__ is defined after the > > EXPORT_SYMBOL() definition, but I'm now getting warnings with > > genksyms: > > > > AS arch/x86/lib/clear_page_64.o > > <stdin>:3:10: warning: "__GENKSYMS__" redefined > > <command-line>: note: this is the location of the previous definition > > > > Oops. > > Do you think the fix below should be sufficient? > > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -314,7 +314,7 @@ > else > cmd_gensymtypes_S = \ > $(getasmexports) | \ > - $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms) > + $(CPP) $(c_flags) -xc - | $(genksyms) > endif # CONFIG_GENDWARFKSYMS > > quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@ EXPORT_SYMBOL() has a different definition when __GENKSYMS__ is defined, so I think with genksyms we actually do need this on the command line. I suppose you could wrap the getasmexports definition in #ifndef __GENKSYMS__ to avoid the warning, or just use __GENDWARFKSYMS__ like you suggested earlier. Sami
On Thu, 20 Mar 2025 at 17:22, Sami Tolvanen <samitolvanen@google.com> wrote: > > On Thu, Mar 20, 2025 at 9:09 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 20 Mar 2025 at 15:48, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Hi Ard, > > > > > > On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > > > > through the compiler to capture the exported symbols. This ensures that > > > > exported symbols such as __ref_stack_chk_guard on x86, which is declared > > > > conditionally, is visible to the tool. > > > > > > > > Otherwise, an error such as the below may be raised, breaking the build > > > > when CONFIG_GENDWARFKSYMS=y > > > > > > > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > > > > > > > Cc: Sami Tolvanen <samitolvanen@google.com> > > > > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > --- > > > > scripts/Makefile.build | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > > > index 993708d11874..7855cdc4e763 100644 > > > > --- a/scripts/Makefile.build > > > > +++ b/scripts/Makefile.build > > > > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > > > > getasmexports = \ > > > > { echo "\#include <linux/kernel.h>" ; \ > > > > echo "\#include <linux/string.h>" ; \ > > > > + echo "\#define __GENKSYMS__" ; \ > > > > echo "\#include <asm/asm-prototypes.h>" ; \ > > > > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > > > > > This works with gendwarfksyms since __GENKSYMS__ is defined after the > > > EXPORT_SYMBOL() definition, but I'm now getting warnings with > > > genksyms: > > > > > > AS arch/x86/lib/clear_page_64.o > > > <stdin>:3:10: warning: "__GENKSYMS__" redefined > > > <command-line>: note: this is the location of the previous definition > > > > > > > Oops. > > > > Do you think the fix below should be sufficient? > > > > --- a/scripts/Makefile.build > > +++ b/scripts/Makefile.build > > @@ -314,7 +314,7 @@ > > else > > cmd_gensymtypes_S = \ > > $(getasmexports) | \ > > - $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms) > > + $(CPP) $(c_flags) -xc - | $(genksyms) > > endif # CONFIG_GENDWARFKSYMS > > > > quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@ > > EXPORT_SYMBOL() has a different definition when __GENKSYMS__ is > defined, so I think with genksyms we actually do need this on the > command line. I suppose you could wrap the getasmexports definition in > #ifndef __GENKSYMS__ to avoid the warning, or just use > __GENDWARFKSYMS__ like you suggested earlier. > Yeah, on second thought, we could just do what Masahiro suggested, and drop the conditional from asm/asm-prototypes.h The issue in question only affects definitions, not declarations, and so having the declaration visible shouldn't be a problem. Ingo, mind dropping this patch again? We'll do the below instead (I'll send out the patch in a minute) --- a/arch/x86/include/asm/asm-prototypes.h +++ b/arch/x86/include/asm/asm-prototypes.h @@ -20,6 +20,6 @@ extern void cmpxchg8b_emu(void); #endif -#if defined(__GENKSYMS__) && defined(CONFIG_STACKPROTECTOR) +#ifdef CONFIG_STACKPROTECTOR extern unsigned long __ref_stack_chk_guard; #endif
* Ard Biesheuvel <ardb@kernel.org> wrote: > On Thu, 20 Mar 2025 at 17:22, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Thu, Mar 20, 2025 at 9:09 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Thu, 20 Mar 2025 at 15:48, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > Hi Ard, > > > > > > > > On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h > > > > > through the compiler to capture the exported symbols. This ensures that > > > > > exported symbols such as __ref_stack_chk_guard on x86, which is declared > > > > > conditionally, is visible to the tool. > > > > > > > > > > Otherwise, an error such as the below may be raised, breaking the build > > > > > when CONFIG_GENDWARFKSYMS=y > > > > > > > > > > <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard' > > > > > > > > > > Cc: Sami Tolvanen <samitolvanen@google.com> > > > > > Reported-by: syzbot+06fd1a3613c50d36129e@syzkaller.appspotmail.com > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > --- > > > > > scripts/Makefile.build | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > > > > index 993708d11874..7855cdc4e763 100644 > > > > > --- a/scripts/Makefile.build > > > > > +++ b/scripts/Makefile.build > > > > > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE > > > > > getasmexports = \ > > > > > { echo "\#include <linux/kernel.h>" ; \ > > > > > echo "\#include <linux/string.h>" ; \ > > > > > + echo "\#define __GENKSYMS__" ; \ > > > > > echo "\#include <asm/asm-prototypes.h>" ; \ > > > > > $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; } > > > > > > > > This works with gendwarfksyms since __GENKSYMS__ is defined after the > > > > EXPORT_SYMBOL() definition, but I'm now getting warnings with > > > > genksyms: > > > > > > > > AS arch/x86/lib/clear_page_64.o > > > > <stdin>:3:10: warning: "__GENKSYMS__" redefined > > > > <command-line>: note: this is the location of the previous definition > > > > > > > > > > Oops. > > > > > > Do you think the fix below should be sufficient? > > > > > > --- a/scripts/Makefile.build > > > +++ b/scripts/Makefile.build > > > @@ -314,7 +314,7 @@ > > > else > > > cmd_gensymtypes_S = \ > > > $(getasmexports) | \ > > > - $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms) > > > + $(CPP) $(c_flags) -xc - | $(genksyms) > > > endif # CONFIG_GENDWARFKSYMS > > > > > > quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@ > > > > EXPORT_SYMBOL() has a different definition when __GENKSYMS__ is > > defined, so I think with genksyms we actually do need this on the > > command line. I suppose you could wrap the getasmexports definition in > > #ifndef __GENKSYMS__ to avoid the warning, or just use > > __GENDWARFKSYMS__ like you suggested earlier. > > > > Yeah, on second thought, we could just do what Masahiro suggested, and > drop the conditional from asm/asm-prototypes.h > > The issue in question only affects definitions, not declarations, and > so having the declaration visible shouldn't be a problem. > > Ingo, mind dropping this patch again? We'll do the below instead (I'll > send out the patch in a minute) Yeah, done & thanks! Ingo
diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 993708d11874..7855cdc4e763 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE getasmexports = \ { echo "\#include <linux/kernel.h>" ; \ echo "\#include <linux/string.h>" ; \ + echo "\#define __GENKSYMS__" ; \ echo "\#include <asm/asm-prototypes.h>" ; \ $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; }