| Message ID | 1558109235-23042-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Accepted |
| Commit | 3a48a91901c516a46a3406ea576798538a8d94d2 |
| Headers | show |
| Series | [v3] kbuild: check uniqueness of module names | expand |
On Sat, May 18, 2019 at 1:10 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > In the recent build test of linux-next, Stephen saw a build error > caused by a broken .tmp_versions/*.mod file: > > https://lkml.org/lkml/2019/5/13/991 > > drivers/net/phy/asix.ko and drivers/net/usb/asix.ko have the same > basename, and there is a race in generating .tmp_versions/asix.mod > > Kbuild has not checked this before, and it suddenly shows up with > obscure error message when this kind of race occurs. > > Non-unique module names cause various sort of problems, but it is > not trivial to catch them by eyes. > > Hence, this script. > > It checks not only real modules, but also built-in modules (i.e. > controlled by tristate CONFIG option, but currently compiled with =y). > Non-unique names for built-in modules also cause problems because > /sys/modules/ would fall over. > > I tested allmodconfig on the latest kernel, and it detected the > following: FYI. "make -j8 allmodconfig all" takes long time to compile (my machine is cheap...) If you want to detect modules with non-unique name quickly, "make -j8 allyesconfig modules" is much faster. > warning: same basename if the following are built as modules: > drivers/regulator/88pm800.ko > drivers/mfd/88pm800.ko > warning: same basename if the following are built as modules: > drivers/gpu/drm/bridge/adv7511/adv7511.ko > drivers/media/i2c/adv7511.ko > warning: same basename if the following are built as modules: > drivers/net/phy/asix.ko > drivers/net/usb/asix.ko > warning: same basename if the following are built as modules: > fs/coda/coda.ko > drivers/media/platform/coda/coda.ko > warning: same basename if the following are built as modules: > drivers/net/phy/realtek.ko > drivers/net/dsa/realtek.ko > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Stephen Rothwell <sfr@canb.auug.org.au> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > > Changes in v3: > - Simplied sed code (Alexander Kapshuk) > > Changes in v2: > - redirect messages to stderr > - use '--' after 'basename -a' > - use '-r' for xargs to cope with empty modules.order/modules.builtin > > Makefile | 1 + > scripts/modules-check.sh | 16 ++++++++++++++++ > 2 files changed, 17 insertions(+) > create mode 100755 scripts/modules-check.sh > > diff --git a/Makefile b/Makefile > index a61a95b..30792fe 100644 > --- a/Makefile > +++ b/Makefile > @@ -1290,6 +1290,7 @@ modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin > $(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order > @$(kecho) ' Building modules, stage 2.'; > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost > + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/modules-check.sh > > modules.builtin: $(vmlinux-dirs:%=%/modules.builtin) > $(Q)$(AWK) '!x[$$0]++' $^ > $(objtree)/modules.builtin > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > new file mode 100755 > index 0000000..2f65953 > --- /dev/null > +++ b/scripts/modules-check.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +set -e > + > +# Check uniqueness of module names > +check_same_name_modules() > +{ > + for m in $(sed 's:.*/::' modules.order modules.builtin | sort | uniq -d) > + do > + echo "warning: same basename if the following are built as modules:" >&2 > + sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin >&2 > + done > +} > + > +check_same_name_modules > -- > 2.7.4 > -- Best Regards Masahiro Yamada
On Sat, May 18, 2019 at 01:07:15AM +0900, Masahiro Yamada wrote: > In the recent build test of linux-next, Stephen saw a build error > caused by a broken .tmp_versions/*.mod file: > > https://lkml.org/lkml/2019/5/13/991 > > drivers/net/phy/asix.ko and drivers/net/usb/asix.ko have the same > basename, and there is a race in generating .tmp_versions/asix.mod > > Kbuild has not checked this before, and it suddenly shows up with > obscure error message when this kind of race occurs. > > Non-unique module names cause various sort of problems, but it is > not trivial to catch them by eyes. > > Hence, this script. > > It checks not only real modules, but also built-in modules (i.e. > controlled by tristate CONFIG option, but currently compiled with =y). > Non-unique names for built-in modules also cause problems because > /sys/modules/ would fall over. > > I tested allmodconfig on the latest kernel, and it detected the > following: > > warning: same basename if the following are built as modules: > drivers/regulator/88pm800.ko > drivers/mfd/88pm800.ko > warning: same basename if the following are built as modules: > drivers/gpu/drm/bridge/adv7511/adv7511.ko > drivers/media/i2c/adv7511.ko > warning: same basename if the following are built as modules: > drivers/net/phy/asix.ko > drivers/net/usb/asix.ko > warning: same basename if the following are built as modules: > fs/coda/coda.ko > drivers/media/platform/coda/coda.ko > warning: same basename if the following are built as modules: > drivers/net/phy/realtek.ko > drivers/net/dsa/realtek.ko > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Stephen Rothwell <sfr@canb.auug.org.au> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hi Masahiro, On Sat, 18 May 2019 01:07:15 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > It checks not only real modules, but also built-in modules (i.e. > controlled by tristate CONFIG option, but currently compiled with =y). > Non-unique names for built-in modules also cause problems because > /sys/modules/ would fall over. > > I tested allmodconfig on the latest kernel, and it detected the > following: A powerpc ppc64_defconfig produces: warning: same basename if the following are built as modules: arch/powerpc/platforms/powermac/nvram.ko drivers/char/nvram.ko Which is a false positive since arch/powerpc/platforms/powermac/Makefile has # CONFIG_NVRAM is an arch. independent tristate symbol, for pmac32 we really # need this to be a bool. Cheat here and pretend CONFIG_NVRAM=m is really # CONFIG_NVRAM=y obj-$(CONFIG_NVRAM:m=y) += nvram.o Which means that this nvram.o will never be built as a module. -- Cheers, Stephen Rothwell
Hi Masahiro, On Sat, 18 May 2019 01:07:15 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > +++ b/scripts/modules-check.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +set -e > + > +# Check uniqueness of module names > +check_same_name_modules() > +{ > + for m in $(sed 's:.*/::' modules.order modules.builtin | sort | uniq -d) > + do > + echo "warning: same basename if the following are built as modules:" >&2 > + sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin >&2 (bringing out my bike shed paint brush :-)) this could be sed -n "/\/$m/s:^kernel/: :p" ... -- Cheers, Stephen Rothwell
Hi Stephen, On Mon, May 20, 2019 at 8:52 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > Hi Masahiro, > > On Sat, 18 May 2019 01:07:15 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > > It checks not only real modules, but also built-in modules (i.e. > > controlled by tristate CONFIG option, but currently compiled with =y). > > Non-unique names for built-in modules also cause problems because > > /sys/modules/ would fall over. > > > > I tested allmodconfig on the latest kernel, and it detected the > > following: > > A powerpc ppc64_defconfig produces: > > warning: same basename if the following are built as modules: > arch/powerpc/platforms/powermac/nvram.ko > drivers/char/nvram.ko > > Which is a false positive since > arch/powerpc/platforms/powermac/Makefile has > > # CONFIG_NVRAM is an arch. independent tristate symbol, for pmac32 we really > # need this to be a bool. Cheat here and pretend CONFIG_NVRAM=m is really > # CONFIG_NVRAM=y > obj-$(CONFIG_NVRAM:m=y) += nvram.o > > Which means that this nvram.o will never be built as a module. Indeed. I thought it was a good idea to check built-in modules, but I do not have a good way to avoid false positives. I think we should not check modules.builtin. Anyway, allmodconfig has a good test coverage. The following is the planned fix. (I folded your sed code.) diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh index 2f659530e1ec..39e8cb36ba19 100755 --- a/scripts/modules-check.sh +++ b/scripts/modules-check.sh @@ -6,10 +6,10 @@ set -e # Check uniqueness of module names check_same_name_modules() { - for m in $(sed 's:.*/::' modules.order modules.builtin | sort | uniq -d) + for m in $(sed 's:.*/::' modules.order | sort | uniq -d) do - echo "warning: same basename if the following are built as modules:" >&2 - sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin >&2 + echo "warning: same module names found:" >&2 + sed -n "/\/$m/s:^kernel/: :p" modules.order >&2 done } > -- > Cheers, > Stephen Rothwell -- Best Regards Masahiro Yamada
On Mon, May 20, 2019 at 8:52 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > Hi Masahiro, > > On Sat, 18 May 2019 01:07:15 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > > It checks not only real modules, but also built-in modules (i.e. > > controlled by tristate CONFIG option, but currently compiled with =y). > > Non-unique names for built-in modules also cause problems because > > /sys/modules/ would fall over. > > > > I tested allmodconfig on the latest kernel, and it detected the > > following: > > A powerpc ppc64_defconfig produces: > > warning: same basename if the following are built as modules: > arch/powerpc/platforms/powermac/nvram.ko > drivers/char/nvram.ko > > Which is a false positive since > arch/powerpc/platforms/powermac/Makefile has > > # CONFIG_NVRAM is an arch. independent tristate symbol, for pmac32 we really > # need this to be a bool. Cheat here and pretend CONFIG_NVRAM=m is really > # CONFIG_NVRAM=y > obj-$(CONFIG_NVRAM:m=y) += nvram.o > > Which means that this nvram.o will never be built as a module. > -- > Cheers, > Stephen Rothwell BTW, arm64 defconfig also produces a false positive: warning: same basename if the following are built as modules: arch/arm64/lib/crc32.ko lib/crc32.ko CONFIG_CRC32 is a tristate option, but ARM64 selects CRC32. So, CRC32 is always =y. We must stop checking modules.builtin soon. Sorry about noises. -- Best Regards Masahiro Yamada
diff --git a/Makefile b/Makefile index a61a95b..30792fe 100644 --- a/Makefile +++ b/Makefile @@ -1290,6 +1290,7 @@ modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin $(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order @$(kecho) ' Building modules, stage 2.'; $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/modules-check.sh modules.builtin: $(vmlinux-dirs:%=%/modules.builtin) $(Q)$(AWK) '!x[$$0]++' $^ > $(objtree)/modules.builtin diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh new file mode 100755 index 0000000..2f65953 --- /dev/null +++ b/scripts/modules-check.sh @@ -0,0 +1,16 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +set -e + +# Check uniqueness of module names +check_same_name_modules() +{ + for m in $(sed 's:.*/::' modules.order modules.builtin | sort | uniq -d) + do + echo "warning: same basename if the following are built as modules:" >&2 + sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin >&2 + done +} + +check_same_name_modules