| Message ID | 20190517042753.25857-1-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kbuild: check uniqueness of module names | expand |
On Fri, May 17, 2019 at 1:29 PM 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: > > 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> > --- One more question popped up. External modules are out of scope of the community, but it is possible that people create an external module that happens to have the same name as an upstream driver. drivers/this/is/upstream/subsystem/foo.ko /home/fred/my/own/external/module/foo.ko Is modprobe confused in this case too? Perhaps, checking for the M= build might be good too...
Hi Masahiro, Thanks for this, looks good to me. Just a nit below. On Fri, 17 May 2019 13:27:53 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > new file mode 100755 > index 000000000000..c875f6eab01e > --- /dev/null > +++ b/scripts/modules-check.sh > @@ -0,0 +1,20 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +set -e > + > +# Check uniqueness of module names > +check_same_name_modules() > +{ > + same_name_modules=$(cat modules.order modules.builtin | \ ^ This trailing '\' is unnecessary after a pipe symbol. > + xargs -r basename -a -- | sort | uniq -d) > + > + for m in $same_name_modules > + do > + echo "warning: same basename if the following are built as modules:" >&2 > + grep -h -e "/$m" modules.order modules.builtin | \ Same here > + sed 's:^kernel/: :' >&2 > + done > +} > + > +check_same_name_modules Reviewed-by: Stephen ROthwell <sfr@canb.auug.org.au>
On Fri, May 17, 2019 at 01:45:11PM +0900, Masahiro Yamada wrote: > On Fri, May 17, 2019 at 1:29 PM 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: > > > > 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> > > --- > > > One more question popped up. > > External modules are out of scope of the community, > but it is possible that people create an external module > that happens to have the same name as an upstream driver. That is their bug, nothing we can do about that :) thanks, greg k-h
On Thu, May 16, 2019 at 10:37 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, May 17, 2019 at 01:45:11PM +0900, Masahiro Yamada wrote: > > On Fri, May 17, 2019 at 1:29 PM 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: > > > > > > 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> > > > --- > > > > > > One more question popped up. > > > > External modules are out of scope of the community, > > but it is possible that people create an external module > > that happens to have the same name as an upstream driver. > > That is their bug, nothing we can do about that :) It's actually not a bug. For external modules it works pretty much as intended. See DEPMOD.D(5): the search directive tells what's the preference among the directories for modules with the same name. depmod respects that order and put the right one into your modules.dep. This allows to put external modules in a different dir and also to make backports of modules from recent to ancient kernels. These modules with the same name are usually the same module, with a different version. Of course what we have here and you are fixing is a different story. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi
On Fri, May 17, 2019 at 8:33 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: > > 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> > --- > > 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 | 20 ++++++++++++++++++++ > 2 files changed, 21 insertions(+) > create mode 100755 scripts/modules-check.sh > > diff --git a/Makefile b/Makefile > index a61a95b6b38f..30792fec7a12 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 000000000000..c875f6eab01e > --- /dev/null > +++ b/scripts/modules-check.sh > @@ -0,0 +1,20 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +set -e > + > +# Check uniqueness of module names > +check_same_name_modules() > +{ > + same_name_modules=$(cat modules.order modules.builtin | \ > + xargs -r basename -a -- | sort | uniq -d) > + > + for m in $same_name_modules > + do > + echo "warning: same basename if the following are built as modules:" >&2 > + grep -h -e "/$m" modules.order modules.builtin | \ > + sed 's:^kernel/: :' >&2 > + done > +} > + > +check_same_name_modules > -- > 2.17.1 > The 'xargs' '-r' flag is a GNU extension. If POSIX compliance is important here, the use of 'cat', 'xargs' and 'basename' may be substituted with that of 'sed' to initialise same_name_modules: sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d 'Sed' may also be used on its own in the 'for' loop instead of as part of a pipeline along with 'grep' to generate the desired output: sed '/\/'$m'/!d;s:^kernel/: :' modules.order modules.builtin Just a suggestion.
On 17/05/2019 10:16, Alexander Kapshuk wrote: [...] > The 'xargs' '-r' flag is a GNU extension. > If POSIX compliance is important here, the use of 'cat', 'xargs' and > 'basename' may be substituted with that of 'sed' to initialise > same_name_modules: > sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d 's!' is TTBOMK also a GNU-extension: sed 's/.*\///' modules.order modules.builtin | sort | uniq -d > 'Sed' may also be used on its own in the 'for' loop instead of as part > of a pipeline along with 'grep' to generate the desired output: > sed '/\/'$m'/!d;s:^kernel/: :' modules.order modules.builtin sed "/\/${m}/!d;s/^kernel\// /" modules.order modules.builtin MfG, Bernd
On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote: > > On 17/05/2019 10:16, Alexander Kapshuk wrote: > [...] > > The 'xargs' '-r' flag is a GNU extension. > > If POSIX compliance is important here, the use of 'cat', 'xargs' and > > 'basename' may be substituted with that of 'sed' to initialise > > same_name_modules: > > sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d > > 's!' is TTBOMK also a GNU-extension: > sed 's/.*\///' modules.order modules.builtin | sort | uniq -d It isn't. Here's an excerpt from the POSIX manpage for 'sed', http://pubs.opengroup.org/onlinepubs/009695399/utilities/sed.html: [2addr]s/BRE/replacement/flags ... Any character other than backslash or <newline> can be used instead of a slash to delimit the BRE and the replacement.... > > > 'Sed' may also be used on its own in the 'for' loop instead of as part > > of a pipeline along with 'grep' to generate the desired output: > > sed '/\/'$m'/!d;s:^kernel/: :' modules.order modules.builtin > > sed "/\/${m}/!d;s/^kernel\// /" modules.order modules.builtin The parameter expansion syntax is redundant here. See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02: The parameter name or symbol can be enclosed in braces, which are optional except for positional parameters with more than one digit or when parameter is a name and is followed by a character that could be interpreted as part of the name. Here's an alternative version using double quotes. sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin > > MfG, > Bernd > -- > Bernd Petrovitsch Email : bernd@petrovitsch.priv.at > LUGA : http://www.luga.at
On 17/05/2019 11:25, Alexander Kapshuk wrote: > On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch > <bernd@petrovitsch.priv.at> wrote: >> >> On 17/05/2019 10:16, Alexander Kapshuk wrote: >> [...] >>> The 'xargs' '-r' flag is a GNU extension. >>> If POSIX compliance is important here, the use of 'cat', 'xargs' and >>> 'basename' may be substituted with that of 'sed' to initialise >>> same_name_modules: >>> sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d >> >> 's!' is TTBOMK also a GNU-extension: >> sed 's/.*\///' modules.order modules.builtin | sort | uniq -d > > It isn't. > Here's an excerpt from the POSIX manpage for 'sed', > http://pubs.opengroup.org/onlinepubs/009695399/utilities/sed.html: > [2addr]s/BRE/replacement/flags > ... Any character other than backslash or <newline> can be used > instead of a slash to delimit the BRE and the replacement.... Oops, yup, sorry for the noise. Don't know anymore where I encountered problems with that in the past .... MfG, Bernd-- "I dislike type abstraction if it has no real reason. And saving on typing is not a good reason - if your typing speed is the main issue when you're coding, you're doing something seriously wrong." - Linus Torvalds
On Fri, May 17, 2019 at 6:25 PM Alexander Kapshuk <alexander.kapshuk@gmail.com> wrote: > > On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch > <bernd@petrovitsch.priv.at> wrote: > > > > On 17/05/2019 10:16, Alexander Kapshuk wrote: > > [...] > > > The 'xargs' '-r' flag is a GNU extension. > > > If POSIX compliance is important here, the use of 'cat', 'xargs' and > > > 'basename' may be substituted with that of 'sed' to initialise > > > same_name_modules: > > > sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d > > > > 's!' is TTBOMK also a GNU-extension: > > sed 's/.*\///' modules.order modules.builtin | sort | uniq -d > > It isn't. > Here's an excerpt from the POSIX manpage for 'sed', > http://pubs.opengroup.org/onlinepubs/009695399/utilities/sed.html: > [2addr]s/BRE/replacement/flags > ... Any character other than backslash or <newline> can be used > instead of a slash to delimit the BRE and the replacement.... > > > > > > 'Sed' may also be used on its own in the 'for' loop instead of as part > > > of a pipeline along with 'grep' to generate the desired output: > > > sed '/\/'$m'/!d;s:^kernel/: :' modules.order modules.builtin > > > > sed "/\/${m}/!d;s/^kernel\// /" modules.order modules.builtin > > The parameter expansion syntax is redundant here. > See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02: > The parameter name or symbol can be enclosed in braces, which are > optional except for positional parameters with more than one digit or > when parameter is a name and is followed by a character that could be > interpreted as part of the name. > > Here's an alternative version using double quotes. > sed "/\/$m/!d;s:^kernel/: :" modules.order modules.builtin Awesome! This is much shorter. I will use it in v3. Thanks.
On Fri, May 17, 2019 at 2:46 PM Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > On Thu, May 16, 2019 at 10:37 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, May 17, 2019 at 01:45:11PM +0900, Masahiro Yamada wrote: > > > On Fri, May 17, 2019 at 1:29 PM 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: > > > > > > > > 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> > > > > --- > > > > > > > > > One more question popped up. > > > > > > External modules are out of scope of the community, > > > but it is possible that people create an external module > > > that happens to have the same name as an upstream driver. > > > > That is their bug, nothing we can do about that :) > > It's actually not a bug. For external modules it works pretty much as > intended. See DEPMOD.D(5): the search directive tells what's the > preference among the directories for modules with the same name. > depmod respects that order and put the right one into your > modules.dep. > > This allows to put external modules in a different dir and also to > make backports of modules from recent to ancient kernels. These > modules with the same name are usually the same module, with a > different version. Of course what we have here and you are fixing is a > different story. OK, so external modules should not be checked. Thanks for the explanation! > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > Lucas De Marchi
On Fri, May 17, 2019 at 2:34 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > Hi Masahiro, > > Thanks for this, looks good to me. Just a nit below. > > On Fri, 17 May 2019 13:27:53 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > > > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > > new file mode 100755 > > index 000000000000..c875f6eab01e > > --- /dev/null > > +++ b/scripts/modules-check.sh > > @@ -0,0 +1,20 @@ > > +#!/bin/sh > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +set -e > > + > > +# Check uniqueness of module names > > +check_same_name_modules() > > +{ > > + same_name_modules=$(cat modules.order modules.builtin | \ > ^ > This trailing '\' is unnecessary after a pipe symbol. With the suggestion from Alexander Kapshuk, the code in v3 became short enough to fit in 80-columns. Anyway, thanks for pointing it out. > > + xargs -r basename -a -- | sort | uniq -d) > > + > > + for m in $same_name_modules > > + do > > + echo "warning: same basename if the following are built as modules:" >&2 > > + grep -h -e "/$m" modules.order modules.builtin | \ > > Same here > > > + sed 's:^kernel/: :' >&2 > > + done > > +} > > + > > +check_same_name_modules > > Reviewed-by: Stephen ROthwell <sfr@canb.auug.org.au> > > -- > Cheers, > Stephen Rothwell
diff --git a/Makefile b/Makefile index a61a95b6b38f..30792fec7a12 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 000000000000..c875f6eab01e --- /dev/null +++ b/scripts/modules-check.sh @@ -0,0 +1,20 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +set -e + +# Check uniqueness of module names +check_same_name_modules() +{ + same_name_modules=$(cat modules.order modules.builtin | \ + xargs -r basename -a -- | sort | uniq -d) + + for m in $same_name_modules + do + echo "warning: same basename if the following are built as modules:" >&2 + grep -h -e "/$m" modules.order modules.builtin | \ + sed 's:^kernel/: :' >&2 + done +} + +check_same_name_modules