* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Nicolas Schier @ 2025-09-16 13:28 UTC (permalink / raw)
To: Alexey Gladkov, Nathan Chancellor
Cc: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez,
linux-kernel, linux-modules, linux-kbuild, Masahiro Yamada
In-Reply-To: <aMlgMkB2nL31K2OB@example.org>
On Tue, Sep 16, 2025 at 03:03:46PM +0200, Alexey Gladkov wrote:
> On Tue, Sep 16, 2025 at 02:42:48PM +0200, Nicolas Schier wrote:
> > > > > > Hi Alexey,
> > > > > >
> > > > > > with this patch applied, I still get a warning from objcpy as Masahiro
> > > > > > and Stephen wrote [1,2]
> > > > > >
> > > > > > SORTTAB vmlinux.unstripped
> > > > > > + sorttable vmlinux.unstripped
> > > > > > + nm -S vmlinux.unstripped
> > > > > > + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> > > > > > + is_enabled CONFIG_KALLSYMS
> > > > > > + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> > > > > > + cmp -s System.map .tmp_vmlinux2.syms
> > > > > > + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> > > > > > # OBJCOPY vmlinux
> > > > > > objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> > > > > > objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
> > > > > >
> > > > > > (arm64, allnoconfig)
> > > > > >
> > > > > > Kind regards,
> > > > > > Nicolas
> > > > > >
> > > > > >
> > > > > > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> > > > > > [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
> > > > > >
> > > > >
> > > > > Hm. I missed that. I need to investigate how to fix this. Nothing comes
> > > > > to mind right now.
> > > >
> > > > Same here. Only thing I could find until now is
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/scripts/link-vmlinux.sh?id=90ceddcb495008ac8ba7a3dce297841efcd7d584
> > > >
> > > > where '2>/dev/null' is appended exactly to prevent this very warning.
> > > > But for me, it doesn't feel good doing that when stripping to vmlinux.
> > >
> > > Yes, that's not a very good approach. It will hide other errors that will
> > > definitely need to be seen. I think the commit you mentioned is actually
> > > incorrect. I think there should be a different solution.
> > >
> > > I think in the case of .modinfo, we can change the flag in the section
> > > since we are going to delete it anyway.
> > >
> > > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > > index dbbe3bf0cf23..9a118b31d0dc 100644
> > > --- a/scripts/Makefile.vmlinux
> > > +++ b/scripts/Makefile.vmlinux
> > > @@ -87,7 +87,8 @@ remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> > > remove-symbols := -w --strip-symbol='__mod_device_table__*'
> > >
> > > quiet_cmd_strip_relocs = OBJCOPY $@
> > > - cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> > > + cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< && \
> > > + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> > > $(remove-symbols) $< $@
> > >
> > > targets += vmlinux
> >
> > Ah, great! I thought we had to fiddle around with linker scripts et al.
> > I needed to use an intermediate file:
> >
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index e2ceeb9e168d..516d51ca634b 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -90,6 +90,9 @@ remove-section-y := .modinfo
> > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> >
> > quiet_cmd_strip_relocs = OBJCOPY $@
> > - cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $< $@
> > + cmd_strip_relocs = set -e; \
> > + trap 'rm $<.noload' EXIT HUP INT; \
> > + $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< $<.noload && \
> > + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $<.noload $@
> >
> > targets += vmlinux
>
> according to man-page:
>
> If you do not specify outfile, objcopy creates a temporary file and
> destructively renames the result with the name of infile.
>
> That is true even in freebsd:
>
> https://man.freebsd.org/cgi/man.cgi?query=objcopy
>
> Do you want to support any other objcopy implementations ?
yeah, it's actually because I dislike modifying vmlinux.unstripped in
the vmlinux rule.
But it may be that Nathan does not see it this way.
--
Nicolas
^ permalink raw reply
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Alexey Gladkov @ 2025-09-16 13:03 UTC (permalink / raw)
To: Nicolas Schier
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aMlbSEnwGOPM39Op@levanger>
On Tue, Sep 16, 2025 at 02:42:48PM +0200, Nicolas Schier wrote:
> > > > > Hi Alexey,
> > > > >
> > > > > with this patch applied, I still get a warning from objcpy as Masahiro
> > > > > and Stephen wrote [1,2]
> > > > >
> > > > > SORTTAB vmlinux.unstripped
> > > > > + sorttable vmlinux.unstripped
> > > > > + nm -S vmlinux.unstripped
> > > > > + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> > > > > + is_enabled CONFIG_KALLSYMS
> > > > > + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> > > > > + cmp -s System.map .tmp_vmlinux2.syms
> > > > > + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> > > > > # OBJCOPY vmlinux
> > > > > objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> > > > > objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
> > > > >
> > > > > (arm64, allnoconfig)
> > > > >
> > > > > Kind regards,
> > > > > Nicolas
> > > > >
> > > > >
> > > > > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> > > > > [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
> > > > >
> > > >
> > > > Hm. I missed that. I need to investigate how to fix this. Nothing comes
> > > > to mind right now.
> > >
> > > Same here. Only thing I could find until now is
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/scripts/link-vmlinux.sh?id=90ceddcb495008ac8ba7a3dce297841efcd7d584
> > >
> > > where '2>/dev/null' is appended exactly to prevent this very warning.
> > > But for me, it doesn't feel good doing that when stripping to vmlinux.
> >
> > Yes, that's not a very good approach. It will hide other errors that will
> > definitely need to be seen. I think the commit you mentioned is actually
> > incorrect. I think there should be a different solution.
> >
> > I think in the case of .modinfo, we can change the flag in the section
> > since we are going to delete it anyway.
> >
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index dbbe3bf0cf23..9a118b31d0dc 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -87,7 +87,8 @@ remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> > remove-symbols := -w --strip-symbol='__mod_device_table__*'
> >
> > quiet_cmd_strip_relocs = OBJCOPY $@
> > - cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> > + cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< && \
> > + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> > $(remove-symbols) $< $@
> >
> > targets += vmlinux
>
> Ah, great! I thought we had to fiddle around with linker scripts et al.
> I needed to use an intermediate file:
>
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index e2ceeb9e168d..516d51ca634b 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -90,6 +90,9 @@ remove-section-y := .modinfo
> remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
>
> quiet_cmd_strip_relocs = OBJCOPY $@
> - cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $< $@
> + cmd_strip_relocs = set -e; \
> + trap 'rm $<.noload' EXIT HUP INT; \
> + $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< $<.noload && \
> + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $<.noload $@
>
> targets += vmlinux
according to man-page:
If you do not specify outfile, objcopy creates a temporary file and
destructively renames the result with the name of infile.
That is true even in freebsd:
https://man.freebsd.org/cgi/man.cgi?query=objcopy
Do you want to support any other objcopy implementations ?
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Nicolas Schier @ 2025-09-16 12:42 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aMlKTPpNXrRW6v_7@example.org>
On Tue, Sep 16, 2025 at 01:30:20PM +0200, Alexey Gladkov wrote:
> On Tue, Sep 16, 2025 at 11:36:54AM +0200, Nicolas Schier wrote:
> > On Tue, Sep 16, 2025 at 09:12:22AM +0200, Alexey Gladkov wrote:
> > > On Mon, Sep 15, 2025 at 07:56:16AM +0200, Nicolas Schier wrote:
> > > > On Mon, Aug 18, 2025 at 06:54:57PM +0200, Alexey Gladkov wrote:
> > > > > From: Masahiro Yamada <masahiroy@kernel.org>
> > > > >
> > > > > Keep the .modinfo section during linking, but strip it from the final
> > > > > vmlinux.
> > > > >
> > > > > Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
> > > > >
> > > > > This change will allow the next commit to extract the .modinfo section
> > > > > from the vmlinux.unstripped intermediate.
> > > > >
> > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > ---
> > > > > include/asm-generic/vmlinux.lds.h | 2 +-
> > > > > scripts/Makefile.vmlinux | 2 +-
> > > > > scripts/mksysmap | 3 +++
> > > > > 3 files changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > > > > index ae2d2359b79e9..cfa63860dfd4c 100644
> > > > > --- a/include/asm-generic/vmlinux.lds.h
> > > > > +++ b/include/asm-generic/vmlinux.lds.h
> > > > > @@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > > > >
> > > > > /* Required sections not related to debugging. */
> > > > > #define ELF_DETAILS \
> > > > > + .modinfo : { *(.modinfo) } \
> > > > > .comment 0 : { *(.comment) } \
> > > > > .symtab 0 : { *(.symtab) } \
> > > > > .strtab 0 : { *(.strtab) } \
> > > > > @@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > > > > *(.discard.*) \
> > > > > *(.export_symbol) \
> > > > > *(.no_trim_symbol) \
> > > > > - *(.modinfo) \
> > > > > /* ld.bfd warns about .gnu.version* even when not emitted */ \
> > > > > *(.gnu.version*) \
> > > > >
> > > > > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > > > > index 4f2d4c3fb7372..e2ceeb9e168d4 100644
> > > > > --- a/scripts/Makefile.vmlinux
> > > > > +++ b/scripts/Makefile.vmlinux
> > > > > @@ -86,7 +86,7 @@ endif
> > > > > # vmlinux
> > > > > # ---------------------------------------------------------------------------
> > > > >
> > > > > -remove-section-y :=
> > > > > +remove-section-y := .modinfo
> > > > > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> > > > >
> > > > > quiet_cmd_strip_relocs = OBJCOPY $@
> > > > > diff --git a/scripts/mksysmap b/scripts/mksysmap
> > > > > index 3accbdb269ac7..a607a0059d119 100755
> > > > > --- a/scripts/mksysmap
> > > > > +++ b/scripts/mksysmap
> > > > > @@ -79,6 +79,9 @@
> > > > > / _SDA_BASE_$/d
> > > > > / _SDA2_BASE_$/d
> > > > >
> > > > > +# MODULE_INFO()
> > > > > +/ __UNIQUE_ID_modinfo[0-9]*$/d
> > > > > +
> > > > > # ---------------------------------------------------------------------------
> > > > > # Ignored patterns
> > > > > # (symbols that contain the pattern are ignored)
> > > > > --
> > > > > 2.50.1
> > > > >
> > > >
> > > > Hi Alexey,
> > > >
> > > > with this patch applied, I still get a warning from objcpy as Masahiro
> > > > and Stephen wrote [1,2]
> > > >
> > > > SORTTAB vmlinux.unstripped
> > > > + sorttable vmlinux.unstripped
> > > > + nm -S vmlinux.unstripped
> > > > + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> > > > + is_enabled CONFIG_KALLSYMS
> > > > + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> > > > + cmp -s System.map .tmp_vmlinux2.syms
> > > > + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> > > > # OBJCOPY vmlinux
> > > > objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> > > > objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
> > > >
> > > > (arm64, allnoconfig)
> > > >
> > > > Kind regards,
> > > > Nicolas
> > > >
> > > >
> > > > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> > > > [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
> > > >
> > >
> > > Hm. I missed that. I need to investigate how to fix this. Nothing comes
> > > to mind right now.
> >
> > Same here. Only thing I could find until now is
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/scripts/link-vmlinux.sh?id=90ceddcb495008ac8ba7a3dce297841efcd7d584
> >
> > where '2>/dev/null' is appended exactly to prevent this very warning.
> > But for me, it doesn't feel good doing that when stripping to vmlinux.
>
> Yes, that's not a very good approach. It will hide other errors that will
> definitely need to be seen. I think the commit you mentioned is actually
> incorrect. I think there should be a different solution.
>
> I think in the case of .modinfo, we can change the flag in the section
> since we are going to delete it anyway.
>
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index dbbe3bf0cf23..9a118b31d0dc 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -87,7 +87,8 @@ remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> remove-symbols := -w --strip-symbol='__mod_device_table__*'
>
> quiet_cmd_strip_relocs = OBJCOPY $@
> - cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> + cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< && \
> + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
> $(remove-symbols) $< $@
>
> targets += vmlinux
Ah, great! I thought we had to fiddle around with linker scripts et al.
I needed to use an intermediate file:
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index e2ceeb9e168d..516d51ca634b 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -90,6 +90,9 @@ remove-section-y := .modinfo
remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
quiet_cmd_strip_relocs = OBJCOPY $@
- cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $< $@
+ cmd_strip_relocs = set -e; \
+ trap 'rm $<.noload' EXIT HUP INT; \
+ $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< $<.noload && \
+ $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $<.noload $@
targets += vmlinux
Kind regards,
Nicolas
^ permalink raw reply related
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Alexey Gladkov @ 2025-09-16 11:30 UTC (permalink / raw)
To: Nicolas Schier
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aMkvtg55F1gJ5feM@levanger>
On Tue, Sep 16, 2025 at 11:36:54AM +0200, Nicolas Schier wrote:
> On Tue, Sep 16, 2025 at 09:12:22AM +0200, Alexey Gladkov wrote:
> > On Mon, Sep 15, 2025 at 07:56:16AM +0200, Nicolas Schier wrote:
> > > On Mon, Aug 18, 2025 at 06:54:57PM +0200, Alexey Gladkov wrote:
> > > > From: Masahiro Yamada <masahiroy@kernel.org>
> > > >
> > > > Keep the .modinfo section during linking, but strip it from the final
> > > > vmlinux.
> > > >
> > > > Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
> > > >
> > > > This change will allow the next commit to extract the .modinfo section
> > > > from the vmlinux.unstripped intermediate.
> > > >
> > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > ---
> > > > include/asm-generic/vmlinux.lds.h | 2 +-
> > > > scripts/Makefile.vmlinux | 2 +-
> > > > scripts/mksysmap | 3 +++
> > > > 3 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > > > index ae2d2359b79e9..cfa63860dfd4c 100644
> > > > --- a/include/asm-generic/vmlinux.lds.h
> > > > +++ b/include/asm-generic/vmlinux.lds.h
> > > > @@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > > >
> > > > /* Required sections not related to debugging. */
> > > > #define ELF_DETAILS \
> > > > + .modinfo : { *(.modinfo) } \
> > > > .comment 0 : { *(.comment) } \
> > > > .symtab 0 : { *(.symtab) } \
> > > > .strtab 0 : { *(.strtab) } \
> > > > @@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > > > *(.discard.*) \
> > > > *(.export_symbol) \
> > > > *(.no_trim_symbol) \
> > > > - *(.modinfo) \
> > > > /* ld.bfd warns about .gnu.version* even when not emitted */ \
> > > > *(.gnu.version*) \
> > > >
> > > > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > > > index 4f2d4c3fb7372..e2ceeb9e168d4 100644
> > > > --- a/scripts/Makefile.vmlinux
> > > > +++ b/scripts/Makefile.vmlinux
> > > > @@ -86,7 +86,7 @@ endif
> > > > # vmlinux
> > > > # ---------------------------------------------------------------------------
> > > >
> > > > -remove-section-y :=
> > > > +remove-section-y := .modinfo
> > > > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> > > >
> > > > quiet_cmd_strip_relocs = OBJCOPY $@
> > > > diff --git a/scripts/mksysmap b/scripts/mksysmap
> > > > index 3accbdb269ac7..a607a0059d119 100755
> > > > --- a/scripts/mksysmap
> > > > +++ b/scripts/mksysmap
> > > > @@ -79,6 +79,9 @@
> > > > / _SDA_BASE_$/d
> > > > / _SDA2_BASE_$/d
> > > >
> > > > +# MODULE_INFO()
> > > > +/ __UNIQUE_ID_modinfo[0-9]*$/d
> > > > +
> > > > # ---------------------------------------------------------------------------
> > > > # Ignored patterns
> > > > # (symbols that contain the pattern are ignored)
> > > > --
> > > > 2.50.1
> > > >
> > >
> > > Hi Alexey,
> > >
> > > with this patch applied, I still get a warning from objcpy as Masahiro
> > > and Stephen wrote [1,2]
> > >
> > > SORTTAB vmlinux.unstripped
> > > + sorttable vmlinux.unstripped
> > > + nm -S vmlinux.unstripped
> > > + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> > > + is_enabled CONFIG_KALLSYMS
> > > + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> > > + cmp -s System.map .tmp_vmlinux2.syms
> > > + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> > > # OBJCOPY vmlinux
> > > objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> > > objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
> > >
> > > (arm64, allnoconfig)
> > >
> > > Kind regards,
> > > Nicolas
> > >
> > >
> > > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> > > [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
> > >
> >
> > Hm. I missed that. I need to investigate how to fix this. Nothing comes
> > to mind right now.
>
> Same here. Only thing I could find until now is
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/scripts/link-vmlinux.sh?id=90ceddcb495008ac8ba7a3dce297841efcd7d584
>
> where '2>/dev/null' is appended exactly to prevent this very warning.
> But for me, it doesn't feel good doing that when stripping to vmlinux.
Yes, that's not a very good approach. It will hide other errors that will
definitely need to be seen. I think the commit you mentioned is actually
incorrect. I think there should be a different solution.
I think in the case of .modinfo, we can change the flag in the section
since we are going to delete it anyway.
diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index dbbe3bf0cf23..9a118b31d0dc 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -87,7 +87,8 @@ remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
remove-symbols := -w --strip-symbol='__mod_device_table__*'
quiet_cmd_strip_relocs = OBJCOPY $@
- cmd_strip_relocs = $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
+ cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< && \
+ $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) \
$(remove-symbols) $< $@
targets += vmlinux
--
Rgrds, legion
^ permalink raw reply related
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Nicolas Schier @ 2025-09-16 9:36 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aMkN1m55vejTii_H@example.org>
On Tue, Sep 16, 2025 at 09:12:22AM +0200, Alexey Gladkov wrote:
> On Mon, Sep 15, 2025 at 07:56:16AM +0200, Nicolas Schier wrote:
> > On Mon, Aug 18, 2025 at 06:54:57PM +0200, Alexey Gladkov wrote:
> > > From: Masahiro Yamada <masahiroy@kernel.org>
> > >
> > > Keep the .modinfo section during linking, but strip it from the final
> > > vmlinux.
> > >
> > > Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
> > >
> > > This change will allow the next commit to extract the .modinfo section
> > > from the vmlinux.unstripped intermediate.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > > include/asm-generic/vmlinux.lds.h | 2 +-
> > > scripts/Makefile.vmlinux | 2 +-
> > > scripts/mksysmap | 3 +++
> > > 3 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > > index ae2d2359b79e9..cfa63860dfd4c 100644
> > > --- a/include/asm-generic/vmlinux.lds.h
> > > +++ b/include/asm-generic/vmlinux.lds.h
> > > @@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > >
> > > /* Required sections not related to debugging. */
> > > #define ELF_DETAILS \
> > > + .modinfo : { *(.modinfo) } \
> > > .comment 0 : { *(.comment) } \
> > > .symtab 0 : { *(.symtab) } \
> > > .strtab 0 : { *(.strtab) } \
> > > @@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > > *(.discard.*) \
> > > *(.export_symbol) \
> > > *(.no_trim_symbol) \
> > > - *(.modinfo) \
> > > /* ld.bfd warns about .gnu.version* even when not emitted */ \
> > > *(.gnu.version*) \
> > >
> > > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > > index 4f2d4c3fb7372..e2ceeb9e168d4 100644
> > > --- a/scripts/Makefile.vmlinux
> > > +++ b/scripts/Makefile.vmlinux
> > > @@ -86,7 +86,7 @@ endif
> > > # vmlinux
> > > # ---------------------------------------------------------------------------
> > >
> > > -remove-section-y :=
> > > +remove-section-y := .modinfo
> > > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> > >
> > > quiet_cmd_strip_relocs = OBJCOPY $@
> > > diff --git a/scripts/mksysmap b/scripts/mksysmap
> > > index 3accbdb269ac7..a607a0059d119 100755
> > > --- a/scripts/mksysmap
> > > +++ b/scripts/mksysmap
> > > @@ -79,6 +79,9 @@
> > > / _SDA_BASE_$/d
> > > / _SDA2_BASE_$/d
> > >
> > > +# MODULE_INFO()
> > > +/ __UNIQUE_ID_modinfo[0-9]*$/d
> > > +
> > > # ---------------------------------------------------------------------------
> > > # Ignored patterns
> > > # (symbols that contain the pattern are ignored)
> > > --
> > > 2.50.1
> > >
> >
> > Hi Alexey,
> >
> > with this patch applied, I still get a warning from objcpy as Masahiro
> > and Stephen wrote [1,2]
> >
> > SORTTAB vmlinux.unstripped
> > + sorttable vmlinux.unstripped
> > + nm -S vmlinux.unstripped
> > + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> > + is_enabled CONFIG_KALLSYMS
> > + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> > + cmp -s System.map .tmp_vmlinux2.syms
> > + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> > # OBJCOPY vmlinux
> > objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> > objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
> >
> > (arm64, allnoconfig)
> >
> > Kind regards,
> > Nicolas
> >
> >
> > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> > [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
> >
>
> Hm. I missed that. I need to investigate how to fix this. Nothing comes
> to mind right now.
Same here. Only thing I could find until now is
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/scripts/link-vmlinux.sh?id=90ceddcb495008ac8ba7a3dce297841efcd7d584
where '2>/dev/null' is appended exactly to prevent this very warning.
But for me, it doesn't feel good doing that when stripping to vmlinux.
--
Nicolas
^ permalink raw reply
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Alexey Gladkov @ 2025-09-16 7:12 UTC (permalink / raw)
To: Nicolas Schier
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aMeqgPVfJcjBLhl8@levanger>
On Mon, Sep 15, 2025 at 07:56:16AM +0200, Nicolas Schier wrote:
> On Mon, Aug 18, 2025 at 06:54:57PM +0200, Alexey Gladkov wrote:
> > From: Masahiro Yamada <masahiroy@kernel.org>
> >
> > Keep the .modinfo section during linking, but strip it from the final
> > vmlinux.
> >
> > Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
> >
> > This change will allow the next commit to extract the .modinfo section
> > from the vmlinux.unstripped intermediate.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> > include/asm-generic/vmlinux.lds.h | 2 +-
> > scripts/Makefile.vmlinux | 2 +-
> > scripts/mksysmap | 3 +++
> > 3 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index ae2d2359b79e9..cfa63860dfd4c 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> >
> > /* Required sections not related to debugging. */
> > #define ELF_DETAILS \
> > + .modinfo : { *(.modinfo) } \
> > .comment 0 : { *(.comment) } \
> > .symtab 0 : { *(.symtab) } \
> > .strtab 0 : { *(.strtab) } \
> > @@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> > *(.discard.*) \
> > *(.export_symbol) \
> > *(.no_trim_symbol) \
> > - *(.modinfo) \
> > /* ld.bfd warns about .gnu.version* even when not emitted */ \
> > *(.gnu.version*) \
> >
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index 4f2d4c3fb7372..e2ceeb9e168d4 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -86,7 +86,7 @@ endif
> > # vmlinux
> > # ---------------------------------------------------------------------------
> >
> > -remove-section-y :=
> > +remove-section-y := .modinfo
> > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> >
> > quiet_cmd_strip_relocs = OBJCOPY $@
> > diff --git a/scripts/mksysmap b/scripts/mksysmap
> > index 3accbdb269ac7..a607a0059d119 100755
> > --- a/scripts/mksysmap
> > +++ b/scripts/mksysmap
> > @@ -79,6 +79,9 @@
> > / _SDA_BASE_$/d
> > / _SDA2_BASE_$/d
> >
> > +# MODULE_INFO()
> > +/ __UNIQUE_ID_modinfo[0-9]*$/d
> > +
> > # ---------------------------------------------------------------------------
> > # Ignored patterns
> > # (symbols that contain the pattern are ignored)
> > --
> > 2.50.1
> >
>
> Hi Alexey,
>
> with this patch applied, I still get a warning from objcpy as Masahiro
> and Stephen wrote [1,2]
>
> SORTTAB vmlinux.unstripped
> + sorttable vmlinux.unstripped
> + nm -S vmlinux.unstripped
> + ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
> + is_enabled CONFIG_KALLSYMS
> + grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
> + cmp -s System.map .tmp_vmlinux2.syms
> + echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
> # OBJCOPY vmlinux
> objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
> objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
>
> (arm64, allnoconfig)
>
> Kind regards,
> Nicolas
>
>
> [1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
> [2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
>
Hm. I missed that. I need to investigate how to fix this. Nothing comes
to mind right now.
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH 2/4] PCI: Add KUnit tests for FIXUP quirks
From: Brian Norris @ 2025-09-15 20:25 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <aMfJCbld_TMHPTbD@google.com>
Hi,
On Mon, Sep 15, 2025 at 08:06:33AM +0000, Tzung-Bi Shih wrote:
> On Fri, Sep 12, 2025 at 03:59:33PM -0700, Brian Norris wrote:
> > +static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
> > + int size, u32 *val)
> > +{
> > + if (PCI_SLOT(devfn) > 0)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + if (where + size > TEST_CONF_SIZE)
> > + return PCIBIOS_BUFFER_TOO_SMALL;
> > +
> > + if (size == 1)
> > + *val = test_readb(test_conf_space + where);
> > + else if (size == 2)
> > + *val = test_readw(test_conf_space + where);
> > + else if (size == 4)
> > + *val = test_readl(test_conf_space + where);
> > +
> > + return PCIBIOS_SUCCESSFUL;
>
> To handle cases where size might be a value other than {1, 2, 4}, would a
> switch statement with a default case be more robust here?
I was patterning based on pci_generic_config_read() and friends, but I
see that those use an 'else' for the last block, where I used an 'else
if'.
I suppose I could switch to 'else'. I'm not sure 'switch/case' is much
better.
> > +static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
> > + int size, u32 val)
> > +{
> > + if (PCI_SLOT(devfn) > 0)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + if (where + size > TEST_CONF_SIZE)
> > + return PCIBIOS_BUFFER_TOO_SMALL;
> > +
> > + if (size == 1)
> > + test_writeb(test_conf_space + where, val);
> > + else if (size == 2)
> > + test_writew(test_conf_space + where, val);
> > + else if (size == 4)
> > + test_writel(test_conf_space + where, val);
> > +
> > + return PCIBIOS_SUCCESSFUL;
>
> Same here.
>
> > +static struct pci_dev *hook_device_early;
> > +static struct pci_dev *hook_device_header;
> > +static struct pci_dev *hook_device_final;
> > +static struct pci_dev *hook_device_enable;
> > +
> > +static void pci_fixup_early_hook(struct pci_dev *pdev)
> > +{
> > + hook_device_early = pdev;
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
> > [...]
> > +static int pci_fixup_test_init(struct kunit *test)
> > +{
> > + hook_device_early = NULL;
> > + hook_device_header = NULL;
> > + hook_device_final = NULL;
> > + hook_device_enable = NULL;
> > +
> > + return 0;
> > +}
>
> FWIW: if the probe is synchronous and the thread is the same task_struct,
> the module level variables can be eliminated by using:
>
> test->priv = kunit_kzalloc(...);
> KUNIT_ASSERT_PTR_NE(...);
>
> And in the hooks, kunit_get_current_test() returns the struct kunit *.
Ah, good suggestion, will give that a shot.
> > +static void pci_fixup_match_test(struct kunit *test)
> > +{
> > + struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > + KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > + test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
> > + KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
>
> The common initialization code can be moved to pci_fixup_test_init().
>
> > + struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
> > +
> > + KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
> > + bridge->ops = &test_ops;
>
> The `bridge` allocation can be moved to .init() too.
>
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
>
> Does it really need to check them? They are just initialized by .init().
Probably not. I wrote these before there were multiple test cases and an
.init() function, and I didn't reconsider them afterward. And they'll be
especially pointless once these move into a kzalloc'd private structure.
Thanks for the suggestions.
Brian
^ permalink raw reply
* Re: [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Brian Norris @ 2025-09-15 18:41 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <aMgZJgU7p57KC0DL@infradead.org>
Hi Christoph,
On Mon, Sep 15, 2025 at 06:48:22AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 12, 2025 at 03:59:31PM -0700, Brian Norris wrote:
> > This series primarily adds support for DECLARE_PCI_FIXUP_*() in modules.
> > There are a few drivers that already use this, and so they are
> > presumably broken when built as modules.
>
> That's a reall bad idea, because it allows random code to insert quirks
> not even bound to the hardware they support.
I see fixups in controller drivers here:
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-keystone.c
drivers/pci/controller/dwc/pcie-qcom.c
drivers/pci/controller/pci-loongson.c
drivers/pci/controller/pci-tegra.c
drivers/pci/controller/pcie-iproc-bcma.c
drivers/pci/controller/pcie-iproc.c
Are any of those somehow wrong?
And if they are not wrong, then is this a good reason to disallow making
these drivers modular? (Yes, few of them are currently modular; but I
don't see why that *must* be the case.)
I agree, as with many kernel features, there are plenty of ways to use
them incorrectly. But I'm just trying to patch over one rough edge about
how to use them incorrectly, and I don't really see why it's such a bad
idea.
> So no, modules should not allow quirks, but the kernel should probably
> be nice enough to fail compilation when someone is attemping that
> instead of silently ignoring the quirks.
Sure, if consensus says we should not support this, I'd definitely like
to make this failure mode more obvious -- likely a build error.
Thanks for your thoughts,
Brian
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Brian Norris @ 2025-09-15 18:34 UTC (permalink / raw)
To: Johannes Berg
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Manivannan Sadhasivam
In-Reply-To: <8e75d6cc3847899ba8d6a0cbd0ef3ac57eabf009.camel@sipsolutions.net>
Hi Johannes,
On Mon, Sep 15, 2025 at 08:33:08AM +0200, Johannes Berg wrote:
> On Fri, 2025-09-12 at 15:59 -0700, Brian Norris wrote:
> > The PCI framework supports "quirks" for PCI devices via several
> > DECLARE_PCI_FIXUP_*() macros. These macros allow arch or driver code to
> > match device IDs to provide customizations or workarounds for broken
> > devices.
> >
> > This mechanism is generally used in code that can only be built into the
> > kernel, but there are a few occasions where this mechanism is used in
> > drivers that can be modules. For example, see commit 574f29036fce ("PCI:
> > iproc: Apply quirk_paxc_bridge() for module as well as built-in").
> >
> > The PCI fixup mechanism only works for built-in code, however, because
> > pci_fixup_device() only scans the ".pci_fixup_*" linker sections found
> > in the main kernel; it never touches modules.
> >
> > Extend the fixup approach to modules.
>
> This _feels_ a bit odd to me - what if you reload a module, should the
> fixup be done twice? Right now this was not possible in a module, which
> is a bit of a gotcha, but at least that's only one for developers, not
> for users (unless someone messes up and puts it into modular code, as in
> the example you gave.)
My assumption was that FIXUPs in modules are only legitimate if they
apply to a dependency chain that involves the module they are built
into. So for example, the fixup could apply to a bridge that is
supported only by the module (driver) in question; or it could apply
only to devices that sit under the controller in question [1].
Everything I see that could potentially be in a module works like this
AFAICT.
To answer your question: no, the fixup should not be done twice, unless
the device is removed and recreated. More below.
[1] The quirks in drivers/pci/controller/dwc/pci-keystone.c look like
this. (Side note: pci-keystone.c cannot be built as a module today.)
> Although, come to think of it, you don't even apply the fixup when the
> module is loaded, so what I just wrote isn't really true. That almost
> seems like an oversight though, now the module has to be loaded before
> the PCI device is enumerated, which is unlikely to happen in practice?
> But then we get the next gotcha - the device is already enumerated, so
> the fixups cannot be applied at the various enumeration stages, and
> you're back to having to load the module before PCI enumeration, which
> could be tricky, or somehow forcing re-enumeration of a given device
> from userspace, but then you're firmly in "gotcha for the user"
> territory again ...
With my assumption above, none of this would really be needed. The
relevant device(s) will only exist after the module is loaded, and they
will go away when the module is gone.
Or am I misreading your problem statements?
> I don't really have any skin in this game, but overall I'd probably
> argue it's better to occasionally have to fix things such as in the
> commit you point out but have a predictable system, than apply things
> from modules.
FWIW, I believe some folks are working on making *more* controller
drivers modular. So this problem will bite more people. (Specifically, I
believe Manivannan was working on
drivers/pci/controller/dwc/pcie-qcom.c, and it has plenty of FIXUPs.)
I also don't think it makes things much less predictable, as long as
developers abide by my above assumption. I think that's a perfectly
reasonable assumption (it's not so different than, say,
MODULE_DEVICE_TABLE), but I could perhaps be convinced otherwise.
> Perhaps it'd be better to extend the section checking infrastructure to
> catch and error out on these sections in modules instead, so we catch it
> at build time, rather than finding things missing at runtime?
Maybe I'm missing something here, but it seems like it'd be pretty easy
to do something like:
#ifdef MODULE
#define DECLARE_PCI_FIXUP_SECTION...) BUILD_BUG()
#else
... real definitions ...
#endif
I'd prefer not doing this though, if we can help it, since I believe
(a) FIXUPs are useful in perfectly reasonable ways for controller
drivers and
(b) controller drivers can potentially be modules (yes, there are some
pitfalls besides $subject).
> And yeah, now I've totally ignored the kunit angle, but ... not sure how
> to combine the two requirements if they are, as I think, conflicting.
Right, either we support FIXUPs in modules, or we should outlaw them.
For kunit: we could still add tests, but just force them to be built-in.
It wouldn't be the first kernel subsystem to need that.
Brian
^ permalink raw reply
* Re: [PATCH v7 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Nicolas Schier @ 2025-09-15 5:56 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <4d53c72293d88b663257a0d723ebf3473a08b374.1755535876.git.legion@kernel.org>
On Mon, Aug 18, 2025 at 06:54:57PM +0200, Alexey Gladkov wrote:
> From: Masahiro Yamada <masahiroy@kernel.org>
>
> Keep the .modinfo section during linking, but strip it from the final
> vmlinux.
>
> Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
>
> This change will allow the next commit to extract the .modinfo section
> from the vmlinux.unstripped intermediate.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> include/asm-generic/vmlinux.lds.h | 2 +-
> scripts/Makefile.vmlinux | 2 +-
> scripts/mksysmap | 3 +++
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index ae2d2359b79e9..cfa63860dfd4c 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -831,6 +831,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
>
> /* Required sections not related to debugging. */
> #define ELF_DETAILS \
> + .modinfo : { *(.modinfo) } \
> .comment 0 : { *(.comment) } \
> .symtab 0 : { *(.symtab) } \
> .strtab 0 : { *(.strtab) } \
> @@ -1044,7 +1045,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> *(.discard.*) \
> *(.export_symbol) \
> *(.no_trim_symbol) \
> - *(.modinfo) \
> /* ld.bfd warns about .gnu.version* even when not emitted */ \
> *(.gnu.version*) \
>
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index 4f2d4c3fb7372..e2ceeb9e168d4 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -86,7 +86,7 @@ endif
> # vmlinux
> # ---------------------------------------------------------------------------
>
> -remove-section-y :=
> +remove-section-y := .modinfo
> remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
>
> quiet_cmd_strip_relocs = OBJCOPY $@
> diff --git a/scripts/mksysmap b/scripts/mksysmap
> index 3accbdb269ac7..a607a0059d119 100755
> --- a/scripts/mksysmap
> +++ b/scripts/mksysmap
> @@ -79,6 +79,9 @@
> / _SDA_BASE_$/d
> / _SDA2_BASE_$/d
>
> +# MODULE_INFO()
> +/ __UNIQUE_ID_modinfo[0-9]*$/d
> +
> # ---------------------------------------------------------------------------
> # Ignored patterns
> # (symbols that contain the pattern are ignored)
> --
> 2.50.1
>
Hi Alexey,
with this patch applied, I still get a warning from objcpy as Masahiro
and Stephen wrote [1,2]
SORTTAB vmlinux.unstripped
+ sorttable vmlinux.unstripped
+ nm -S vmlinux.unstripped
+ ./scripts/sorttable -s .tmp_vmlinux.nm-sort vmlinux.unstripped
+ is_enabled CONFIG_KALLSYMS
+ grep -q ^CONFIG_KALLSYMS=y include/config/auto.conf
+ cmp -s System.map .tmp_vmlinux2.syms
+ echo vmlinux.unstripped: ../scripts/link-vmlinux.sh
# OBJCOPY vmlinux
objcopy --remove-section=.modinfo vmlinux.unstripped vmlinux
objcopy: vmlinux.unstripped: warning: empty loadable segment detected at vaddr=0xffff8000807a0000, is this intentional?
(arm64, allnoconfig)
Kind regards,
Nicolas
[1]: https://lore.kernel.org/linux-kbuild/CAK7LNAR-gD2H6Kk-rZjo0R3weTHCGTm0a=u2tRH1WWW6Sx6=RQ@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/20250730164047.7c4a731a@canb.auug.org.au/
^ permalink raw reply
* Re: [PATCH v7 6/8] modpost: Add modname to mod_device_table alias
From: Nicolas Schier @ 2025-09-14 20:07 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Miguel Ojeda, Andreas Hindborg, Danilo Krummrich, Alex Gaynor,
rust-for-linux
In-Reply-To: <66785b63b47878446a15bcb14a9ef42dc3bda092.1755535876.git.legion@kernel.org>
On Mon, Aug 18, 2025 at 06:55:00PM +0200, Alexey Gladkov wrote:
> At this point, if a symbol is compiled as part of the kernel,
> information about which module the symbol belongs to is lost.
>
> To save this it is possible to add the module name to the alias name.
> It's not very pretty, but it's possible for now.
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Andreas Hindborg <a.hindborg@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Alex Gaynor <alex.gaynor@gmail.com>
> Cc: rust-for-linux@vger.kernel.org
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> ---
> include/linux/module.h | 14 +++++++++++++-
> rust/kernel/device_id.rs | 8 ++++----
> scripts/mod/file2alias.c | 18 ++++++++++++++----
> 3 files changed, 31 insertions(+), 9 deletions(-)
>
...
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 00586119a25b7..13021266a18f8 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1476,8 +1476,8 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> {
> void *symval;
> char *zeros = NULL;
> - const char *type, *name;
> - size_t typelen;
> + const char *type, *name, *modname;
> + size_t typelen, modnamelen;
when applying the patch-set onto kbuild-next, gcc refuses to build this patch
(this possibly killing future bisecting):
../scripts/mod/file2alias.c: In function ‘handle_moddevtable’:
../scripts/mod/file2alias.c:1480:25: error: variable ‘modnamelen’ set but not used [-Werror=unused-but-set-variable]
1480 | size_t typelen, modnamelen;
| ^~~~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [../scripts/Makefile.host:131: scripts/mod/file2alias.o] Error
(-Werror is on for userprogs, since commit
27758d8c2583d10472b745a43ff86fef96c11ef7)
Introduction of modnamelen has to be moved to the next patch.
Kind regards
Nicolas
^ permalink raw reply
* Re: [RFC PATCH 00/10] scalable symbol flags with __kflagstab
From: Sid Nayyar @ 2025-09-15 15:53 UTC (permalink / raw)
To: Petr Pavlu
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel, Giuliano Procida,
Matthias Männich
In-Reply-To: <409ddefc-24f8-465c-8872-17dc585626a6@suse.com>
On Mon, Sep 8, 2025 at 11:09 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
> This sounds reasonable to me. Do you have any numbers on hand that would
> show the impact of extending __ksymtab?
I did performance analysis for module loading. The kflagstab
optimizes symbol search, which accounts for less than 2% of the
average module load time. Therefore, this change does not translate
into any meaningful gains (or losses) in module loading performance.
On the binary size side, the on-disk size for vmlinux is somewhat
inflated due to extra entries in .symtab and .strtab. Since these
sections are not part of the final Image, the only increase in the
in-memory size of the kernel is for the kflagstab itself. This new
table occupies 1 byte for each symbol in the ksymtab.
> > The Android Common Kernel source is compiled into what we call
> > GKI (Generic Kernel Image), which consists of a kernel and a
> > number of modules. We maintain a stable interface (based on CRCs and
> > types) between the GKI components and vendor-specific modules
> > (compiled by device manufacturers, e.g., for hardware-specific
> > drivers) for the lifetime of a given GKI version.
> >
> > This interface is intentionally restricted to the minimal set of
> > symbols required by the union of all vendor modules; our partners
> > declare their requirements in symbol lists. Any additions to these
> > lists are reviewed to ensure kernel internals are not overly exposed.
> > For example, we restrict drivers from having the ability to open and
> > read arbitrary files. This ABI boundary also allows us to evolve
> > internal kernel types that are not exposed to vendor modules, for
> > example, when a security fix requires a type to change.
> >
> > The mechanism we use for this is CONFIG_TRIM_UNUSED_KSYMS and
> > CONFIG_UNUSED_KSYMS_WHITELIST. This results in a ksymtab
> > containing two kinds of exported symbols: those explicitly required
> > by vendors ("vendor-listed") and those only required by GKI modules
> > ("GKI use only").
> >
> > On top of this, we have implemented symbol import protection
> > (covered in patches 9/10 and 10/10). This feature prevents vendor
> > modules from using symbols that are not on the vendor-listed
> > whitelist. It is built on top of CONFIG_MODULE_SIG. GKI modules are
> > signed with a specific key, while vendor modules are unsigned and thus
> > treated as untrusted. This distinction allows signed GKI modules to
> > use any symbol in the ksymtab, while unsigned vendor modules can only
> > access the declared subset. This provides a significant layer of
> > defense and security against potentially exploitable vendor module
> > code.
>
> If I understand correctly, this is similar to the recently introduced
> EXPORT_SYMBOL_FOR_MODULES() macro, but with a coarser boundary.
>
> I think that if the goal is to control the kABI scope and limit the use
> of certain symbols only to GKI modules, then having the protection
> depend on whether the module is signed is somewhat odd. It doesn't give
> me much confidence if vendor modules are unsigned in the Android
> ecosystem. I would expect that you want to improve this in the long
> term.
GKI modules are the only modules built in the same Kbuild as the
kernel image, which Google builds and provides to partners. In
contrast, vendor modules are built and packaged entirely by partners.
Google signs GKI modules with ephemeral keys. Since partners do
not have these keys, vendor modules are treated as unsigned by
the kernel.
To ensure the authenticity of these unsigned modules, partners
package them into a separate image that becomes one of the boot
partitions. This entire image is signed, and its signature is
verified by the bootloader at boot time.
> It would then make more sense to me if the protection was determined by
> whether the module is in-tree (the "intree" flag in modinfo) or,
> alternatively, if it is signed by a built-in trusted key. I feel this
> way the feature could be potentially useful for other distributions that
> care about the kABI scope and have ecosystems where vendor modules are
> properly signed with some key. However, I'm not sure if this would still
> work in your case.
Partners can produce both in-tree and out-of-tree modules. We do not
trust either type regarding symbol exposure, as there is no way to know
exactly what sources were used. Furthermore, symbols exported via
EXPORT_SYMBOL_FOR_MODULES can be accessed by any vendor module that
mimics the GKI module name.
Therefore, neither the in-tree flag nor the EXPORT_SYMBOL_FOR_MODULES
mechanism provides a strong enough guarantee for the Android kernel to
identify GKI modules.
Only module signatures are sufficient to allow a module to access the
full set of exported symbols. Unsigned vendor modules may only access
the symbol subset declared ahead of time by partners.
In case such symbol protection is not useful for the Linux community, I
am happy to keep this as an Android-specific feature. However, I would
urge you to at least accept the kflagstab, as it allows us (and
potentially other Linux distributions) to easily introduce additional
flags for symbols. It is also a simplification/clean-up of the module
loader code.
--
Thanks,
Siddharth Nayyar
^ permalink raw reply
* Re: [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Christoph Hellwig @ 2025-09-15 13:48 UTC (permalink / raw)
To: Brian Norris
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <20250912230208.967129-1-briannorris@chromium.org>
On Fri, Sep 12, 2025 at 03:59:31PM -0700, Brian Norris wrote:
> This series primarily adds support for DECLARE_PCI_FIXUP_*() in modules.
> There are a few drivers that already use this, and so they are
> presumably broken when built as modules.
That's a reall bad idea, because it allows random code to insert quirks
not even bound to the hardware they support.
So no, modules should not allow quirks, but the kernel should probably
be nice enough to fail compilation when someone is attemping that
instead of silently ignoring the quirks.
^ permalink raw reply
* Re: [PATCH v2] kernel/module: avoid panic when loading corrupt module
From: Petr Pavlu @ 2025-09-15 12:27 UTC (permalink / raw)
To: Daniel v. Kirschten
Cc: mcgrof, Daniel Gomez, Sami Tolvanen, linux-modules, linux-kernel
In-Reply-To: <d1d1756a-2f6d-4b81-bd6d-50ddf7f39996@gmail.com>
On 9/9/25 6:46 PM, Daniel v. Kirschten wrote:
> If the kernel attempts loading a corrupted module where the
> .gnu.linkonce.this_module section is not marked as WRITE,
> the corresponding this_module struct will be remapped read-only
> in the module loading process. This causes a kernel panic later -
> specifically the first time that struct is being written to after the remap.
> (Currently, this happens in complete_formation at kernel/module/main.c:3265,
> when the module state is set to COMING,
> but this doesn't really matter and of course might also change in the future.)
>
> This panic also causes problems down the line:
> after this panic has occurred, all further attempts
> to add or remove modules will freeze the process attempting to do so.
> I did not investigate this further.
>
> The kernel's module building toolchain will not produce such module files.
> However, there's only a single bit difference on-disk
> between a correct module file and one which triggers this panic.
> Also, there are modules which aren't produced by the kernel's module toolchain.
> (Yes, we don't necessarily need to fully support such modules,
> but we shouldn't panic when loading them either.)
>
> Note that from a security point of view, this bug is irrelevant:
> the problematic remap of this_module as readonly
> only happens after all security checks have already passed.
> If an attacker is in the position to insert arbitrary modules into the kernel,
> then it doesn't matter anymore that it's possible to cause a panic too.
> And even in the hypothetical scenario where an attacker
> can trigger this panic but can't insert custom modules,
> the worst that could happen is a DoS
> caused by future module insertion / removal attempts.
>
> Signed-off-by: Daniel Kirschten <danielkirschten@gmail.com>
Nits:
I suggest making the subject of the patch more specific to avoid
potential confusion. For instance:
module: Check that .gnu.linkonce.this_module is writable
I think the description could also be a bit more straightforward and the
text should ideally be reflown to a 75-column boundary (see
Documentation/process/submitting-patches.rst).
> ---
>
> I hope that the wording is clear enough now about this not being a security bug.
> What do you think?
>
> In my first submisison of this patch (on 06/06/2024),
> I was told to add this check to userspace kmod too.
> If I find the time, I will do so, but I think that won't help as much
> because there are of course other ways for userspace to load a module,
> such as any alternative insmod implementation (for example busybox).
>
> Regarding your "next-level university assignment":
> I feel knowing whether the kernel toolchain can or cannot produce such modules
> is a bit beside the point: _if_ such a module is encountered,
> the kernel's going to panic, and it's not going to care where the module came from.
> Also I'm a bit stumped by your wording "university assignment" here.
> Still, I recognize that it would be goot to be certain
> that the official tools don't produce broken modules.
> So, I debugged the module build system a bit and found out the following:
>
> add_header in scripts/mod/modpost.c:1834-1843 is responsible
> for arranging for the .gnu.linkonce.this_module section to exist:
> The C code this function emits defines the symbol __this_module
> with two attributes: `visibility("default")` and `section(".gnu.linkonce.this_module")`.
> In particular, __this_module is not marked const or anything similar,
> and there definitely is no (supported) way
> for the user to add custom modifiers to this symbol.
> When gcc compiles that file, the resulting section is marked WRITE and ALLOC.
> This seems to be default behaviour of gcc / ld,
> but I couldn't find explicit documentation about this.
> I even tried digging in gcc's source code to find hard proof,
> but as expected gcc turns out to be quite convoluted, so eventually I gave up.
>
> kernel/module/main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..c415c58b9462 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2092,6 +2092,12 @@ static int elf_validity_cache_index_mod(struct load_info *info)
> return -ENOEXEC;
> }
>
> + if (!(shdr->sh_flags & SHF_WRITE)) {
> + pr_err("module %s: .gnu.linkonce.this_module must be writable\n",
> + info->name ?: "(missing .modinfo section or name field)");
> + return -ENOEXEC;
> + }
> +
> if (shdr->sh_size != sizeof(struct module)) {
> pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
> info->name ?: "(missing .modinfo section or name field)");
This looks ok to me and in line with other checks in
elf_validity_cache_index_mod(), although we should be careful about the
number of ELF sanity checks performed by the module loader.
I think the important part is that the module loader should always at
least get through the signature and blacklist checks without crashing
due to a corrupted ELF file. After that point, the module content is
to be trusted, but we try to error out for most issues that would cause
problems later on. The added check is useful for the latter.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 2/4] PCI: Add KUnit tests for FIXUP quirks
From: Tzung-Bi Shih @ 2025-09-15 8:06 UTC (permalink / raw)
To: Brian Norris
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <20250912230208.967129-3-briannorris@chromium.org>
On Fri, Sep 12, 2025 at 03:59:33PM -0700, Brian Norris wrote:
> +static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 *val)
> +{
> + if (PCI_SLOT(devfn) > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + if (where + size > TEST_CONF_SIZE)
> + return PCIBIOS_BUFFER_TOO_SMALL;
> +
> + if (size == 1)
> + *val = test_readb(test_conf_space + where);
> + else if (size == 2)
> + *val = test_readw(test_conf_space + where);
> + else if (size == 4)
> + *val = test_readl(test_conf_space + where);
> +
> + return PCIBIOS_SUCCESSFUL;
To handle cases where size might be a value other than {1, 2, 4}, would a
switch statement with a default case be more robust here?
> +static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 val)
> +{
> + if (PCI_SLOT(devfn) > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + if (where + size > TEST_CONF_SIZE)
> + return PCIBIOS_BUFFER_TOO_SMALL;
> +
> + if (size == 1)
> + test_writeb(test_conf_space + where, val);
> + else if (size == 2)
> + test_writew(test_conf_space + where, val);
> + else if (size == 4)
> + test_writel(test_conf_space + where, val);
> +
> + return PCIBIOS_SUCCESSFUL;
Same here.
> +static struct pci_dev *hook_device_early;
> +static struct pci_dev *hook_device_header;
> +static struct pci_dev *hook_device_final;
> +static struct pci_dev *hook_device_enable;
> +
> +static void pci_fixup_early_hook(struct pci_dev *pdev)
> +{
> + hook_device_early = pdev;
> +}
> +DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
> [...]
> +static int pci_fixup_test_init(struct kunit *test)
> +{
> + hook_device_early = NULL;
> + hook_device_header = NULL;
> + hook_device_final = NULL;
> + hook_device_enable = NULL;
> +
> + return 0;
> +}
FWIW: if the probe is synchronous and the thread is the same task_struct,
the module level variables can be eliminated by using:
test->priv = kunit_kzalloc(...);
KUNIT_ASSERT_PTR_NE(...);
And in the hooks, kunit_get_current_test() returns the struct kunit *.
> +static void pci_fixup_match_test(struct kunit *test)
> +{
> + struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> + KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> + test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
> + KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
The common initialization code can be moved to pci_fixup_test_init().
> + struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
> +
> + KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
> + bridge->ops = &test_ops;
The `bridge` allocation can be moved to .init() too.
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
Does it really need to check them? They are just initialized by .init().
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Johannes Berg @ 2025-09-15 6:33 UTC (permalink / raw)
To: Brian Norris, Bjorn Helgaas, Luis Chamberlain, Petr Pavlu,
Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <20250912230208.967129-2-briannorris@chromium.org>
On Fri, 2025-09-12 at 15:59 -0700, Brian Norris wrote:
> The PCI framework supports "quirks" for PCI devices via several
> DECLARE_PCI_FIXUP_*() macros. These macros allow arch or driver code to
> match device IDs to provide customizations or workarounds for broken
> devices.
>
> This mechanism is generally used in code that can only be built into the
> kernel, but there are a few occasions where this mechanism is used in
> drivers that can be modules. For example, see commit 574f29036fce ("PCI:
> iproc: Apply quirk_paxc_bridge() for module as well as built-in").
>
> The PCI fixup mechanism only works for built-in code, however, because
> pci_fixup_device() only scans the ".pci_fixup_*" linker sections found
> in the main kernel; it never touches modules.
>
> Extend the fixup approach to modules.
This _feels_ a bit odd to me - what if you reload a module, should the
fixup be done twice? Right now this was not possible in a module, which
is a bit of a gotcha, but at least that's only one for developers, not
for users (unless someone messes up and puts it into modular code, as in
the example you gave.)
Although, come to think of it, you don't even apply the fixup when the
module is loaded, so what I just wrote isn't really true. That almost
seems like an oversight though, now the module has to be loaded before
the PCI device is enumerated, which is unlikely to happen in practice?
But then we get the next gotcha - the device is already enumerated, so
the fixups cannot be applied at the various enumeration stages, and
you're back to having to load the module before PCI enumeration, which
could be tricky, or somehow forcing re-enumeration of a given device
from userspace, but then you're firmly in "gotcha for the user"
territory again ...
I don't really have any skin in this game, but overall I'd probably
argue it's better to occasionally have to fix things such as in the
commit you point out but have a predictable system, than apply things
from modules.
Perhaps it'd be better to extend the section checking infrastructure to
catch and error out on these sections in modules instead, so we catch it
at build time, rather than finding things missing at runtime?
And yeah, now I've totally ignored the kunit angle, but ... not sure how
to combine the two requirements if they are, as I think, conflicting.
johannes
^ permalink raw reply
* [PATCH 4/4] kunit: qemu_configs: Add PCI to arm, arm64
From: Brian Norris @ 2025-09-12 22:59 UTC (permalink / raw)
To: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Brian Norris
In-Reply-To: <20250912230208.967129-1-briannorris@chromium.org>
To get some more test coverage on PCI tests.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
tools/testing/kunit/qemu_configs/arm.py | 1 +
tools/testing/kunit/qemu_configs/arm64.py | 1 +
2 files changed, 2 insertions(+)
diff --git a/tools/testing/kunit/qemu_configs/arm.py b/tools/testing/kunit/qemu_configs/arm.py
index db2160200566..101d67e5157c 100644
--- a/tools/testing/kunit/qemu_configs/arm.py
+++ b/tools/testing/kunit/qemu_configs/arm.py
@@ -3,6 +3,7 @@ from ..qemu_config import QemuArchParams
QEMU_ARCH = QemuArchParams(linux_arch='arm',
kconfig='''
CONFIG_ARCH_VIRT=y
+CONFIG_PCI=y
CONFIG_SERIAL_AMBA_PL010=y
CONFIG_SERIAL_AMBA_PL010_CONSOLE=y
CONFIG_SERIAL_AMBA_PL011=y
diff --git a/tools/testing/kunit/qemu_configs/arm64.py b/tools/testing/kunit/qemu_configs/arm64.py
index 5c44d3a87e6d..ba2b4e660ba7 100644
--- a/tools/testing/kunit/qemu_configs/arm64.py
+++ b/tools/testing/kunit/qemu_configs/arm64.py
@@ -2,6 +2,7 @@ from ..qemu_config import QemuArchParams
QEMU_ARCH = QemuArchParams(linux_arch='arm64',
kconfig='''
+CONFIG_PCI=y
CONFIG_SERIAL_AMBA_PL010=y
CONFIG_SERIAL_AMBA_PL010_CONSOLE=y
CONFIG_SERIAL_AMBA_PL011=y
--
2.51.0.384.g4c02a37b29-goog
^ permalink raw reply related
* [PATCH 3/4] um: Select PCI_DOMAINS_GENERIC
From: Brian Norris @ 2025-09-12 22:59 UTC (permalink / raw)
To: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Brian Norris
In-Reply-To: <20250912230208.967129-1-briannorris@chromium.org>
This is useful especially for KUnit tests, where we may want to
dynamically add/remove PCI domains.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
arch/um/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 9083bfdb7735..7fccd63c3229 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -38,6 +38,7 @@ config UML
select HAVE_ARCH_TRACEHOOK
select HAVE_SYSCALL_TRACEPOINTS
select THREAD_INFO_IN_TASK
+ select PCI_DOMAINS_GENERIC if PCI
config MMU
bool
--
2.51.0.384.g4c02a37b29-goog
^ permalink raw reply related
* [PATCH 2/4] PCI: Add KUnit tests for FIXUP quirks
From: Brian Norris @ 2025-09-12 22:59 UTC (permalink / raw)
To: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Brian Norris
In-Reply-To: <20250912230208.967129-1-briannorris@chromium.org>
The PCI framework supports device quirks via a series of macros and
linker sections. This support previously did not work when used in
modules. Add a basic set of tests for matching/non-matching devices.
Example run:
$ ./tools/testing/kunit/kunit.py run 'pci_fixup*'
[...]
[15:31:30] ============ pci_fixup_test_cases (2 subtests) =============
[15:31:30] [PASSED] pci_fixup_match_test
[15:31:30] [PASSED] pci_fixup_nomatch_test
[15:31:30] ============== [PASSED] pci_fixup_test_cases ===============
[15:31:30] ============================================================
[15:31:30] Testing complete. Ran 2 tests: passed: 2
[15:31:30] Elapsed time: 11.197s total, 0.001s configuring, 9.870s building, 1.299s running
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/pci/Kconfig | 11 +++
drivers/pci/Makefile | 1 +
drivers/pci/fixup-test.c | 197 +++++++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/pci/fixup-test.c
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 9a249c65aedc..a4fa9be797e7 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -68,6 +68,17 @@ config PCI_QUIRKS
Disable this only if your target machine is unaffected by PCI
quirks.
+config PCI_FIXUP_KUNIT_TEST
+ tristate "KUnit tests for PCI fixup code" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ depends on PCI_DOMAINS_GENERIC
+ default KUNIT_ALL_TESTS
+ help
+ This builds unit tests for the PCI quirk/fixup framework. Recommended
+ only for kernel developers.
+
+ When in doubt, say N.
+
config PCI_DEBUG
bool "PCI Debugging"
depends on DEBUG_KERNEL
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 67647f1880fb..ade400250ceb 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -20,6 +20,7 @@ endif
obj-$(CONFIG_OF) += of.o
obj-$(CONFIG_PCI_QUIRKS) += quirks.o
+obj-$(CONFIG_PCI_FIXUP_KUNIT_TEST) += fixup-test.o
obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
obj-$(CONFIG_PCI_ATS) += ats.o
obj-$(CONFIG_PCI_IOV) += iov.o
diff --git a/drivers/pci/fixup-test.c b/drivers/pci/fixup-test.c
new file mode 100644
index 000000000000..54b895fc8f3e
--- /dev/null
+++ b/drivers/pci/fixup-test.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google, Inc.
+ */
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+#include <linux/cleanup.h>
+#include <linux/pci.h>
+
+#define DEVICE_NAME "pci_fixup_test_device"
+#define TEST_VENDOR_ID 0xdead
+#define TEST_DEVICE_ID 0xbeef
+
+#define TEST_CONF_SIZE 4096
+static u8 *test_conf_space;
+
+#define test_readb(addr) (*(u8 *)(addr))
+#define test_readw(addr) le16_to_cpu(*((__force __le16 *)(addr)))
+#define test_readl(addr) le32_to_cpu(*((__force __le32 *)(addr)))
+#define test_writeb(addr, v) (*(u8 *)(addr) = (v))
+#define test_writew(addr, v) (*((__force __le16 *)(addr)) = cpu_to_le16(v))
+#define test_writel(addr, v) (*((__force __le32 *)(addr)) = cpu_to_le32(v))
+
+static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 *val)
+{
+ if (PCI_SLOT(devfn) > 0)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ if (where + size > TEST_CONF_SIZE)
+ return PCIBIOS_BUFFER_TOO_SMALL;
+
+ if (size == 1)
+ *val = test_readb(test_conf_space + where);
+ else if (size == 2)
+ *val = test_readw(test_conf_space + where);
+ else if (size == 4)
+ *val = test_readl(test_conf_space + where);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 val)
+{
+ if (PCI_SLOT(devfn) > 0)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ if (where + size > TEST_CONF_SIZE)
+ return PCIBIOS_BUFFER_TOO_SMALL;
+
+ if (size == 1)
+ test_writeb(test_conf_space + where, val);
+ else if (size == 2)
+ test_writew(test_conf_space + where, val);
+ else if (size == 4)
+ test_writel(test_conf_space + where, val);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops test_ops = {
+ .read = test_config_read,
+ .write = test_config_write,
+};
+
+static struct pci_dev *hook_device_early;
+static struct pci_dev *hook_device_header;
+static struct pci_dev *hook_device_final;
+static struct pci_dev *hook_device_enable;
+
+static void pci_fixup_early_hook(struct pci_dev *pdev)
+{
+ hook_device_early = pdev;
+}
+DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
+
+static void pci_fixup_header_hook(struct pci_dev *pdev)
+{
+ hook_device_header = pdev;
+}
+DECLARE_PCI_FIXUP_HEADER(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_header_hook);
+
+static void pci_fixup_final_hook(struct pci_dev *pdev)
+{
+ hook_device_final = pdev;
+}
+DECLARE_PCI_FIXUP_FINAL(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_final_hook);
+
+static void pci_fixup_enable_hook(struct pci_dev *pdev)
+{
+ hook_device_enable = pdev;
+}
+DECLARE_PCI_FIXUP_ENABLE(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_enable_hook);
+
+static int pci_fixup_test_init(struct kunit *test)
+{
+ hook_device_early = NULL;
+ hook_device_header = NULL;
+ hook_device_final = NULL;
+ hook_device_enable = NULL;
+
+ return 0;
+}
+
+static void pci_fixup_match_test(struct kunit *test)
+{
+ struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+ test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
+
+ /* Minimal configuration space: a stub vendor and device ID */
+ test_writew(test_conf_space + PCI_VENDOR_ID, TEST_VENDOR_ID);
+ test_writew(test_conf_space + PCI_DEVICE_ID, TEST_DEVICE_ID);
+
+ struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
+ bridge->ops = &test_ops;
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_host_probe(bridge));
+
+ struct pci_dev *pdev __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+ KUNIT_ASSERT_PTR_NE(test, NULL, pdev);
+
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_enable_device(pdev));
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_enable);
+}
+
+static void pci_fixup_nomatch_test(struct kunit *test)
+{
+ struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+ test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
+
+ /* Minimal configuration space: a stub vendor and device ID */
+ test_writew(test_conf_space + PCI_VENDOR_ID, TEST_VENDOR_ID + 1);
+ test_writew(test_conf_space + PCI_DEVICE_ID, TEST_DEVICE_ID);
+
+ struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
+ bridge->ops = &test_ops;
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_host_probe(bridge));
+
+ struct pci_dev *pdev __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+ KUNIT_ASSERT_PTR_NE(test, NULL, pdev);
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_enable_device(pdev));
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+}
+
+static struct kunit_case pci_fixup_test_cases[] = {
+ KUNIT_CASE(pci_fixup_match_test),
+ KUNIT_CASE(pci_fixup_nomatch_test),
+ {}
+};
+
+static struct kunit_suite pci_fixup_test_suite = {
+ .name = "pci_fixup_test_cases",
+ .test_cases = pci_fixup_test_cases,
+ .init = pci_fixup_test_init,
+};
+
+kunit_test_suite(pci_fixup_test_suite);
+MODULE_DESCRIPTION("PCI fixups unit test suite");
+MODULE_LICENSE("GPL");
--
2.51.0.384.g4c02a37b29-goog
^ permalink raw reply related
* [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Brian Norris @ 2025-09-12 22:59 UTC (permalink / raw)
To: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Brian Norris
In-Reply-To: <20250912230208.967129-1-briannorris@chromium.org>
The PCI framework supports "quirks" for PCI devices via several
DECLARE_PCI_FIXUP_*() macros. These macros allow arch or driver code to
match device IDs to provide customizations or workarounds for broken
devices.
This mechanism is generally used in code that can only be built into the
kernel, but there are a few occasions where this mechanism is used in
drivers that can be modules. For example, see commit 574f29036fce ("PCI:
iproc: Apply quirk_paxc_bridge() for module as well as built-in").
The PCI fixup mechanism only works for built-in code, however, because
pci_fixup_device() only scans the ".pci_fixup_*" linker sections found
in the main kernel; it never touches modules.
Extend the fixup approach to modules.
I don't attempt to be clever here; the algorithm here scales with the
number of modules in the system.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/pci/quirks.c | 62 ++++++++++++++++++++++++++++++++++++++++++
include/linux/module.h | 18 ++++++++++++
kernel/module/main.c | 26 ++++++++++++++++++
3 files changed, 106 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d97335a40193..db5e0ac82ed7 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -207,6 +207,62 @@ extern struct pci_fixup __end_pci_fixups_suspend_late[];
static bool pci_apply_fixup_final_quirks;
+struct pci_fixup_arg {
+ struct pci_dev *dev;
+ enum pci_fixup_pass pass;
+};
+
+static int pci_module_fixup(struct module *mod, void *parm)
+{
+ struct pci_fixup_arg *arg = parm;
+ void *start;
+ unsigned int size;
+
+ switch (arg->pass) {
+ case pci_fixup_early:
+ start = mod->pci_fixup_early;
+ size = mod->pci_fixup_early_size;
+ break;
+ case pci_fixup_header:
+ start = mod->pci_fixup_header;
+ size = mod->pci_fixup_header_size;
+ break;
+ case pci_fixup_final:
+ start = mod->pci_fixup_final;
+ size = mod->pci_fixup_final_size;
+ break;
+ case pci_fixup_enable:
+ start = mod->pci_fixup_enable;
+ size = mod->pci_fixup_enable_size;
+ break;
+ case pci_fixup_resume:
+ start = mod->pci_fixup_resume;
+ size = mod->pci_fixup_resume_size;
+ break;
+ case pci_fixup_suspend:
+ start = mod->pci_fixup_suspend;
+ size = mod->pci_fixup_suspend_size;
+ break;
+ case pci_fixup_resume_early:
+ start = mod->pci_fixup_resume_early;
+ size = mod->pci_fixup_resume_early_size;
+ break;
+ case pci_fixup_suspend_late:
+ start = mod->pci_fixup_suspend_late;
+ size = mod->pci_fixup_suspend_late_size;
+ break;
+ default:
+ return 0;
+ }
+
+ if (!size)
+ return 0;
+
+ pci_do_fixups(arg->dev, start, start + size);
+
+ return 0;
+}
+
void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
{
struct pci_fixup *start, *end;
@@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
return;
}
pci_do_fixups(dev, start, end);
+
+ struct pci_fixup_arg arg = {
+ .dev = dev,
+ .pass = pass,
+ };
+ module_for_each_mod(pci_module_fixup, &arg);
}
EXPORT_SYMBOL(pci_fixup_device);
diff --git a/include/linux/module.h b/include/linux/module.h
index 3319a5269d28..7faa8987b9eb 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -539,6 +539,24 @@ struct module {
int num_kunit_suites;
struct kunit_suite **kunit_suites;
#endif
+#ifdef CONFIG_PCI_QUIRKS
+ void *pci_fixup_early;
+ unsigned int pci_fixup_early_size;
+ void *pci_fixup_header;
+ unsigned int pci_fixup_header_size;
+ void *pci_fixup_final;
+ unsigned int pci_fixup_final_size;
+ void *pci_fixup_enable;
+ unsigned int pci_fixup_enable_size;
+ void *pci_fixup_resume;
+ unsigned int pci_fixup_resume_size;
+ void *pci_fixup_suspend;
+ unsigned int pci_fixup_suspend_size;
+ void *pci_fixup_resume_early;
+ unsigned int pci_fixup_resume_early_size;
+ void *pci_fixup_suspend_late;
+ unsigned int pci_fixup_suspend_late_size;
+#endif
#ifdef CONFIG_LIVEPATCH
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..50a80c875adc 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2702,6 +2702,32 @@ static int find_module_sections(struct module *mod, struct load_info *info)
sizeof(*mod->kunit_init_suites),
&mod->num_kunit_init_suites);
#endif
+#ifdef CONFIG_PCI_QUIRKS
+ mod->pci_fixup_early = section_objs(info, ".pci_fixup_early",
+ sizeof(*mod->pci_fixup_early),
+ &mod->pci_fixup_early_size);
+ mod->pci_fixup_header = section_objs(info, ".pci_fixup_header",
+ sizeof(*mod->pci_fixup_header),
+ &mod->pci_fixup_header_size);
+ mod->pci_fixup_final = section_objs(info, ".pci_fixup_final",
+ sizeof(*mod->pci_fixup_final),
+ &mod->pci_fixup_final_size);
+ mod->pci_fixup_enable = section_objs(info, ".pci_fixup_enable",
+ sizeof(*mod->pci_fixup_enable),
+ &mod->pci_fixup_enable_size);
+ mod->pci_fixup_resume = section_objs(info, ".pci_fixup_resume",
+ sizeof(*mod->pci_fixup_resume),
+ &mod->pci_fixup_resume_size);
+ mod->pci_fixup_suspend = section_objs(info, ".pci_fixup_suspend",
+ sizeof(*mod->pci_fixup_suspend),
+ &mod->pci_fixup_suspend_size);
+ mod->pci_fixup_resume_early = section_objs(info, ".pci_fixup_resume_early",
+ sizeof(*mod->pci_fixup_resume_early),
+ &mod->pci_fixup_resume_early_size);
+ mod->pci_fixup_suspend_late = section_objs(info, ".pci_fixup_suspend_late",
+ sizeof(*mod->pci_fixup_suspend_late),
+ &mod->pci_fixup_suspend_late_size);
+#endif
mod->extable = section_objs(info, "__ex_table",
sizeof(*mod->extable), &mod->num_exentries);
--
2.51.0.384.g4c02a37b29-goog
^ permalink raw reply related
* [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Brian Norris @ 2025-09-12 22:59 UTC (permalink / raw)
To: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez
Cc: linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um,
Brian Norris
This series primarily adds support for DECLARE_PCI_FIXUP_*() in modules.
There are a few drivers that already use this, and so they are
presumably broken when built as modules.
While at it, I wrote some unit tests that emulate a fake PCI device, and
let the PCI framework match/not-match its vendor/device IDs. This test
can be built into the kernel or built as a module.
I also include some infrastructure changes (patch 3 and 4), so that
ARCH=um (the default for kunit.py), ARCH=arm, and ARCH=arm64 will run
these tests by default. These patches have different maintainers and are
independent, so they can probably be picked up separately. I included
them because otherwise the tests in patch 2 aren't so easy to run.
Brian Norris (4):
PCI: Support FIXUP quirks in modules
PCI: Add KUnit tests for FIXUP quirks
um: Select PCI_DOMAINS_GENERIC
kunit: qemu_configs: Add PCI to arm, arm64
arch/um/Kconfig | 1 +
drivers/pci/Kconfig | 11 ++
drivers/pci/Makefile | 1 +
drivers/pci/fixup-test.c | 197 ++++++++++++++++++++++
drivers/pci/quirks.c | 62 +++++++
include/linux/module.h | 18 ++
kernel/module/main.c | 26 +++
tools/testing/kunit/qemu_configs/arm.py | 1 +
tools/testing/kunit/qemu_configs/arm64.py | 1 +
9 files changed, 318 insertions(+)
create mode 100644 drivers/pci/fixup-test.c
--
2.51.0.384.g4c02a37b29-goog
^ permalink raw reply
* Re: [RFC] crypto: support for a standalone FIPS 140 module
From: Herbert Xu @ 2025-09-11 5:53 UTC (permalink / raw)
To: Vegard Nossum
Cc: David S. Miller, linux-crypto, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Ard Biesheuvel, Eric Biggers, Jason A . Donenfeld,
Greg Kroah-Hartman, Wang, Jay, Nicolai Stange, Vladis Dronov,
Stephan Mueller, Sami Tolvanen, linux-modules
In-Reply-To: <20250904155216.460962-1-vegard.nossum@oracle.com>
On Thu, Sep 04, 2025 at 05:50:32PM +0200, Vegard Nossum wrote:
> Hi all,
>
> This patch set adds support for building and loading a standalone FIPS
> 140 module. This is mostly useful for distributions that want to certify
> their kernel's crypto code with NIST. Please see
> Documentation/crypto/fips140.rst for more details.
>
> I apologize for the large patch series. I could have squashed
> it down to fewer commits but it would really make it harder to see
> what's going on.
Perhaps we can divide this by layer? The public key crypto sits
on top of the Crypto API, which in turns sits on top of lib/crypto.
So it would seem natural to divide this into three parts. The
code in lib/crypto can be converted without affecting anything on
top of it, and then the Crypto API could be converted.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH RFC 005/104] crypto: hide crypto_default_rng variable
From: Herbert Xu @ 2025-09-11 5:48 UTC (permalink / raw)
To: Vegard Nossum
Cc: David S. Miller, linux-crypto, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Ard Biesheuvel, Eric Biggers, Jason A . Donenfeld,
Greg Kroah-Hartman, Wang, Jay, Nicolai Stange, Vladis Dronov,
Stephan Mueller, Sami Tolvanen, linux-modules,
Sriharsha Yadagudde
In-Reply-To: <20250904155216.460962-6-vegard.nossum@oracle.com>
On Thu, Sep 04, 2025 at 05:50:37PM +0200, Vegard Nossum wrote:
>
> -int crypto_get_default_rng(void)
> +int crypto_get_default_rng(struct crypto_rng **result)
The usual way to do this is
struct crypto_rng *crypto_get_default_rng(void)
and return the error value as an ERR_PTR().
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH v2] kernel/module: avoid panic when loading corrupt module
From: Daniel v. Kirschten @ 2025-09-09 16:46 UTC (permalink / raw)
To: mcgrof; +Cc: linux-modules, linux-kernel
If the kernel attempts loading a corrupted module where the
.gnu.linkonce.this_module section is not marked as WRITE,
the corresponding this_module struct will be remapped read-only
in the module loading process. This causes a kernel panic later -
specifically the first time that struct is being written to after the remap.
(Currently, this happens in complete_formation at kernel/module/main.c:3265,
when the module state is set to COMING,
but this doesn't really matter and of course might also change in the future.)
This panic also causes problems down the line:
after this panic has occurred, all further attempts
to add or remove modules will freeze the process attempting to do so.
I did not investigate this further.
The kernel's module building toolchain will not produce such module files.
However, there's only a single bit difference on-disk
between a correct module file and one which triggers this panic.
Also, there are modules which aren't produced by the kernel's module toolchain.
(Yes, we don't necessarily need to fully support such modules,
but we shouldn't panic when loading them either.)
Note that from a security point of view, this bug is irrelevant:
the problematic remap of this_module as readonly
only happens after all security checks have already passed.
If an attacker is in the position to insert arbitrary modules into the kernel,
then it doesn't matter anymore that it's possible to cause a panic too.
And even in the hypothetical scenario where an attacker
can trigger this panic but can't insert custom modules,
the worst that could happen is a DoS
caused by future module insertion / removal attempts.
Signed-off-by: Daniel Kirschten <danielkirschten@gmail.com>
---
I hope that the wording is clear enough now about this not being a security bug.
What do you think?
In my first submisison of this patch (on 06/06/2024),
I was told to add this check to userspace kmod too.
If I find the time, I will do so, but I think that won't help as much
because there are of course other ways for userspace to load a module,
such as any alternative insmod implementation (for example busybox).
Regarding your "next-level university assignment":
I feel knowing whether the kernel toolchain can or cannot produce such modules
is a bit beside the point: _if_ such a module is encountered,
the kernel's going to panic, and it's not going to care where the module came from.
Also I'm a bit stumped by your wording "university assignment" here.
Still, I recognize that it would be goot to be certain
that the official tools don't produce broken modules.
So, I debugged the module build system a bit and found out the following:
add_header in scripts/mod/modpost.c:1834-1843 is responsible
for arranging for the .gnu.linkonce.this_module section to exist:
The C code this function emits defines the symbol __this_module
with two attributes: `visibility("default")` and `section(".gnu.linkonce.this_module")`.
In particular, __this_module is not marked const or anything similar,
and there definitely is no (supported) way
for the user to add custom modifiers to this symbol.
When gcc compiles that file, the resulting section is marked WRITE and ALLOC.
This seems to be default behaviour of gcc / ld,
but I couldn't find explicit documentation about this.
I even tried digging in gcc's source code to find hard proof,
but as expected gcc turns out to be quite convoluted, so eventually I gave up.
kernel/module/main.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..c415c58b9462 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2092,6 +2092,12 @@ static int elf_validity_cache_index_mod(struct load_info *info)
return -ENOEXEC;
}
+ if (!(shdr->sh_flags & SHF_WRITE)) {
+ pr_err("module %s: .gnu.linkonce.this_module must be writable\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
if (shdr->sh_size != sizeof(struct module)) {
pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
info->name ?: "(missing .modinfo section or name field)");
--
2.39.5
^ permalink raw reply related
* Re: [PATCH] module : fix signature checker pointer arithmetic and bounds check
From: Luis Chamberlain @ 2025-09-09 16:45 UTC (permalink / raw)
To: Christophe Leroy
Cc: Fidal Palamparambil, linux-modules, petr.pavlu, da.gomez,
samitolvanen, linux-kernel
In-Reply-To: <d46498e5-db21-4a79-93b4-3869be3660d2@csgroup.eu>
On Tue, Sep 09, 2025 at 11:32:27AM +0200, Christophe Leroy wrote:
>
>
> Le 05/09/2025 à 17:45, Fidal Palamparambil a écrit :
> > From: Fidal palamparambil <rootuserhere@gmail.com>
> >
> > This patch fixes :
> > - invalid module_param type (bool_enable_only → bool)
>
> Can you explain what the problem is ? Why do you say bool_enable_only is
> invalid ? Was generalised by commit d19f05d8a8fa ("kernel/params.c:
> generalize bool_enable_only")
Christophe, I will try to save you time. You can ignore the submitter's patch.
They are a troll. I recommend you add them to your ignore list.
Luis
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox