| Message ID | 20190515073818.22486-1-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] kbuild: check uniqueness of basename of modules | expand |
On Wed, May 15, 2019 at 4:40 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 occasionally shows up with > obscure error message when this kind of race occurs. > > It is not trivial to catch this potential issue by eyes. > > Hence, this script. > > I compile-tested allmodconfig for the latest kernel as of writing, > it detected the following: > > warning: same basename '88pm800.ko' if the following are built as modules: > drivers/regulator/88pm800.ko > drivers/mfd/88pm800.ko > warning: same basename 'adv7511.ko' if the following are built as modules: > drivers/gpu/drm/bridge/adv7511/adv7511.ko > drivers/media/i2c/adv7511.ko > warning: same basename 'asix.ko' if the following are built as modules: > drivers/net/phy/asix.ko > drivers/net/usb/asix.ko > warning: same basename 'coda.ko' if the following are built as modules: > fs/coda/coda.ko > drivers/media/platform/coda/coda.ko > warning: same basename 'realtek.ko' 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> > --- > > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > new file mode 100755 > index 000000000000..944e68bd22b0 > --- /dev/null > +++ b/scripts/modules-check.sh > @@ -0,0 +1,18 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +# Warn if two or more modules have the same basename > +check_same_name_modules() > +{ > + same_name_modules=$(cat modules.order modules.builtin | \ > + xargs basename -a | sort | uniq -d) > + > + for m in $same_name_modules > + do > + echo "warning: same basename '$m' if the following are built as modules:" > + grep --no-filename -e /$m modules.order modules.builtin | \ > + sed 's:^kernel/: :' Self nit-picking: It might be better to send these messages to stderr instead of stdout. -- Best Regards Masahiro Yamada
On Wed, May 15, 2019 at 9:39 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 occasionally shows up with > obscure error message when this kind of race occurs. > > It is not trivial to catch this potential issue by eyes. > > Hence, this script. > > I compile-tested allmodconfig for the latest kernel as of writing, > it detected the following: > > warning: same basename '88pm800.ko' if the following are built as modules: > drivers/regulator/88pm800.ko > drivers/mfd/88pm800.ko > warning: same basename 'adv7511.ko' if the following are built as modules: > drivers/gpu/drm/bridge/adv7511/adv7511.ko > drivers/media/i2c/adv7511.ko > warning: same basename 'asix.ko' if the following are built as modules: > drivers/net/phy/asix.ko > drivers/net/usb/asix.ko > warning: same basename 'coda.ko' if the following are built as modules: > fs/coda/coda.ko > drivers/media/platform/coda/coda.ko > warning: same basename 'realtek.ko' 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> That looks great! > --- > > [Alternative fix ? ] > > I do not know about the user experience of modprobe etc. > when two different modules have the same name. > It does not matter if this is correctly handled by modules.order? > > If this is just a problem of the build system, it is pretty easy to fix. > For example, if we prepend the directory path, parallel build will > never write to the same file simultanously. > > asix.mod -> drivers/net/phy/asix.mod > asix.mod -> drivers/net/usb/asix.mod non-unique module names cause all kinds of problems, and modprobe can certainly not handle them correctly, and there are issues with symbols exported from a module when another one has the same name. Arnd
On Wed, May 15, 2019 at 10:08:12AM +0200, Arnd Bergmann wrote: > On Wed, May 15, 2019 at 9:39 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 occasionally shows up with > > obscure error message when this kind of race occurs. > > > > It is not trivial to catch this potential issue by eyes. > > > > Hence, this script. > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > it detected the following: > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > drivers/regulator/88pm800.ko > > drivers/mfd/88pm800.ko > > warning: same basename 'adv7511.ko' if the following are built as modules: > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > drivers/media/i2c/adv7511.ko > > warning: same basename 'asix.ko' if the following are built as modules: > > drivers/net/phy/asix.ko > > drivers/net/usb/asix.ko > > warning: same basename 'coda.ko' if the following are built as modules: > > fs/coda/coda.ko > > drivers/media/platform/coda/coda.ko > > warning: same basename 'realtek.ko' 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> > > That looks great! > > > --- > > > > [Alternative fix ? ] > > > > I do not know about the user experience of modprobe etc. > > when two different modules have the same name. > > It does not matter if this is correctly handled by modules.order? > > > > If this is just a problem of the build system, it is pretty easy to fix. > > For example, if we prepend the directory path, parallel build will > > never write to the same file simultanously. > > > > asix.mod -> drivers/net/phy/asix.mod > > asix.mod -> drivers/net/usb/asix.mod > > non-unique module names cause all kinds of problems, and > modprobe can certainly not handle them correctly, and there > are issues with symbols exported from a module when another > one has the same name. /sys/modules/ will fall over when this happens as well. I thought we had the "rule" that module names had to be unique, I guess it was only a matter of time before they started colliding :( So warning is good, but forbidding this is better, as things will break. Or we need to fix up the places where things will break. thanks, greg k-h
On Wed, May 15, 2019 at 5:14 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, May 15, 2019 at 10:08:12AM +0200, Arnd Bergmann wrote: > > On Wed, May 15, 2019 at 9:39 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 occasionally shows up with > > > obscure error message when this kind of race occurs. > > > > > > It is not trivial to catch this potential issue by eyes. > > > > > > Hence, this script. > > > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > > it detected the following: > > > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > > drivers/regulator/88pm800.ko > > > drivers/mfd/88pm800.ko > > > warning: same basename 'adv7511.ko' if the following are built as modules: > > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > > drivers/media/i2c/adv7511.ko > > > warning: same basename 'asix.ko' if the following are built as modules: > > > drivers/net/phy/asix.ko > > > drivers/net/usb/asix.ko > > > warning: same basename 'coda.ko' if the following are built as modules: > > > fs/coda/coda.ko > > > drivers/media/platform/coda/coda.ko > > > warning: same basename 'realtek.ko' 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> > > > > That looks great! > > > > > --- > > > > > > [Alternative fix ? ] > > > > > > I do not know about the user experience of modprobe etc. > > > when two different modules have the same name. > > > It does not matter if this is correctly handled by modules.order? > > > > > > If this is just a problem of the build system, it is pretty easy to fix. > > > For example, if we prepend the directory path, parallel build will > > > never write to the same file simultanously. > > > > > > asix.mod -> drivers/net/phy/asix.mod > > > asix.mod -> drivers/net/usb/asix.mod > > > > non-unique module names cause all kinds of problems, and > > modprobe can certainly not handle them correctly, and there > > are issues with symbols exported from a module when another > > one has the same name. > > /sys/modules/ will fall over when this happens as well. I thought we > had the "rule" that module names had to be unique, I guess it was only a > matter of time before they started colliding :( > > So warning is good, but forbidding this is better, as things will break. > > Or we need to fix up the places where things will break. If we intentionally break the build, we need to send fix-up patches to subsystems first. -- Best Regards Masahiro Yamada
On Wed, May 15, 2019 at 05:57:50PM +0900, Masahiro Yamada wrote: > On Wed, May 15, 2019 at 5:14 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Wed, May 15, 2019 at 10:08:12AM +0200, Arnd Bergmann wrote: > > > On Wed, May 15, 2019 at 9:39 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 occasionally shows up with > > > > obscure error message when this kind of race occurs. > > > > > > > > It is not trivial to catch this potential issue by eyes. > > > > > > > > Hence, this script. > > > > > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > > > it detected the following: > > > > > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > > > drivers/regulator/88pm800.ko > > > > drivers/mfd/88pm800.ko > > > > warning: same basename 'adv7511.ko' if the following are built as modules: > > > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > > > drivers/media/i2c/adv7511.ko > > > > warning: same basename 'asix.ko' if the following are built as modules: > > > > drivers/net/phy/asix.ko > > > > drivers/net/usb/asix.ko > > > > warning: same basename 'coda.ko' if the following are built as modules: > > > > fs/coda/coda.ko > > > > drivers/media/platform/coda/coda.ko > > > > warning: same basename 'realtek.ko' 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> > > > > > > That looks great! > > > > > > > --- > > > > > > > > [Alternative fix ? ] > > > > > > > > I do not know about the user experience of modprobe etc. > > > > when two different modules have the same name. > > > > It does not matter if this is correctly handled by modules.order? > > > > > > > > If this is just a problem of the build system, it is pretty easy to fix. > > > > For example, if we prepend the directory path, parallel build will > > > > never write to the same file simultanously. > > > > > > > > asix.mod -> drivers/net/phy/asix.mod > > > > asix.mod -> drivers/net/usb/asix.mod > > > > > > non-unique module names cause all kinds of problems, and > > > modprobe can certainly not handle them correctly, and there > > > are issues with symbols exported from a module when another > > > one has the same name. > > > > /sys/modules/ will fall over when this happens as well. I thought we > > had the "rule" that module names had to be unique, I guess it was only a > > matter of time before they started colliding :( > > > > So warning is good, but forbidding this is better, as things will break. > > > > Or we need to fix up the places where things will break. > > > If we intentionally break the build, > we need to send fix-up patches to subsystems first. True, but those builds are already broken if anyone tries to use them :) A warning for now would be nice, that way we can find these and know to fix them up over time. thanks, greg k-h
On Wed, May 15, 2019 at 8:34 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, May 15, 2019 at 05:57:50PM +0900, Masahiro Yamada wrote: > > On Wed, May 15, 2019 at 5:14 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, May 15, 2019 at 10:08:12AM +0200, Arnd Bergmann wrote: > > > > On Wed, May 15, 2019 at 9:39 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 occasionally shows up with > > > > > obscure error message when this kind of race occurs. > > > > > > > > > > It is not trivial to catch this potential issue by eyes. > > > > > > > > > > Hence, this script. > > > > > > > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > > > > it detected the following: > > > > > > > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > > > > drivers/regulator/88pm800.ko > > > > > drivers/mfd/88pm800.ko > > > > > warning: same basename 'adv7511.ko' if the following are built as modules: > > > > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > > > > drivers/media/i2c/adv7511.ko > > > > > warning: same basename 'asix.ko' if the following are built as modules: > > > > > drivers/net/phy/asix.ko > > > > > drivers/net/usb/asix.ko > > > > > warning: same basename 'coda.ko' if the following are built as modules: > > > > > fs/coda/coda.ko > > > > > drivers/media/platform/coda/coda.ko > > > > > warning: same basename 'realtek.ko' 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> > > > > > > > > That looks great! > > > > > > > > > --- > > > > > > > > > > [Alternative fix ? ] > > > > > > > > > > I do not know about the user experience of modprobe etc. > > > > > when two different modules have the same name. > > > > > It does not matter if this is correctly handled by modules.order? > > > > > > > > > > If this is just a problem of the build system, it is pretty easy to fix. > > > > > For example, if we prepend the directory path, parallel build will > > > > > never write to the same file simultanously. > > > > > > > > > > asix.mod -> drivers/net/phy/asix.mod > > > > > asix.mod -> drivers/net/usb/asix.mod > > > > > > > > non-unique module names cause all kinds of problems, and > > > > modprobe can certainly not handle them correctly, and there > > > > are issues with symbols exported from a module when another > > > > one has the same name. > > > > > > /sys/modules/ will fall over when this happens as well. I thought we > > > had the "rule" that module names had to be unique, I guess it was only a > > > matter of time before they started colliding :( > > > > > > So warning is good, but forbidding this is better, as things will break. > > > > > > Or we need to fix up the places where things will break. > > > > > > If we intentionally break the build, > > we need to send fix-up patches to subsystems first. > > True, but those builds are already broken if anyone tries to use them :) > > A warning for now would be nice, that way we can find these and know to > fix them up over time. Yes, I think it is a fair point. Start this with warning, then people will soon notice the problem. Turning it into error is easy once we fix all instances. -- Best Regards Masahiro Yamada
On Wed, May 15, 2019 at 04:53:15PM +0900, Masahiro Yamada wrote: > On Wed, May 15, 2019 at 4:40 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 occasionally shows up with > > obscure error message when this kind of race occurs. > > > > It is not trivial to catch this potential issue by eyes. > > > > Hence, this script. > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > it detected the following: > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > drivers/regulator/88pm800.ko > > drivers/mfd/88pm800.ko > > warning: same basename 'adv7511.ko' if the following are built as modules: > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > drivers/media/i2c/adv7511.ko > > warning: same basename 'asix.ko' if the following are built as modules: > > drivers/net/phy/asix.ko > > drivers/net/usb/asix.ko > > warning: same basename 'coda.ko' if the following are built as modules: > > fs/coda/coda.ko > > drivers/media/platform/coda/coda.ko > > warning: same basename 'realtek.ko' 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> > > --- > > > > > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > > new file mode 100755 > > index 000000000000..944e68bd22b0 > > --- /dev/null > > +++ b/scripts/modules-check.sh > > @@ -0,0 +1,18 @@ > > +#!/bin/sh > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +# Warn if two or more modules have the same basename > > +check_same_name_modules() > > +{ > > + same_name_modules=$(cat modules.order modules.builtin | \ > > + xargs basename -a | sort | uniq -d) While probably it'll never be a problem, just for robustness, I'd add "--" to the end basename to terminate argument interpretation: xargs basename -a -- | sort | ... > > + > > + for m in $same_name_modules > > + do > > + echo "warning: same basename '$m' if the following are built as modules:" > > + grep --no-filename -e /$m modules.order modules.builtin | \ > > + sed 's:^kernel/: :' > > > Self nit-picking: > It might be better to send these messages to stderr instead of stdout. Yeah, wrapping a ">&2" around the report would be good. With these fixes: Reviewed-by: Kees Cook <keescook@chromium.org> -- Kees Cook
Hi Kees, On Thu, May 16, 2019 at 1:20 AM Kees Cook <keescook@chromium.org> wrote: > > On Wed, May 15, 2019 at 04:53:15PM +0900, Masahiro Yamada wrote: > > On Wed, May 15, 2019 at 4:40 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 occasionally shows up with > > > obscure error message when this kind of race occurs. > > > > > > It is not trivial to catch this potential issue by eyes. > > > > > > Hence, this script. > > > > > > I compile-tested allmodconfig for the latest kernel as of writing, > > > it detected the following: > > > > > > warning: same basename '88pm800.ko' if the following are built as modules: > > > drivers/regulator/88pm800.ko > > > drivers/mfd/88pm800.ko > > > warning: same basename 'adv7511.ko' if the following are built as modules: > > > drivers/gpu/drm/bridge/adv7511/adv7511.ko > > > drivers/media/i2c/adv7511.ko > > > warning: same basename 'asix.ko' if the following are built as modules: > > > drivers/net/phy/asix.ko > > > drivers/net/usb/asix.ko > > > warning: same basename 'coda.ko' if the following are built as modules: > > > fs/coda/coda.ko > > > drivers/media/platform/coda/coda.ko > > > warning: same basename 'realtek.ko' 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> > > > --- > > > > > > > > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh > > > new file mode 100755 > > > index 000000000000..944e68bd22b0 > > > --- /dev/null > > > +++ b/scripts/modules-check.sh > > > @@ -0,0 +1,18 @@ > > > +#!/bin/sh > > > +# SPDX-License-Identifier: GPL-2.0 > > > + > > > +# Warn if two or more modules have the same basename > > > +check_same_name_modules() > > > +{ > > > + same_name_modules=$(cat modules.order modules.builtin | \ > > > + xargs basename -a | sort | uniq -d) > > While probably it'll never be a problem, just for robustness, I'd add "--" > to the end basename to terminate argument interpretation: > > xargs basename -a -- | sort | ... Sorry for my ignorance, but could you teach me the effect of "--" ? I sometimes use "--" as a separator when there is ambiguity in arguments for example, "git log <revision> -- <path>" In this case, what is intended by "--"? -- Best Regards Masahiro Yamada
On Wed, May 15, 2019 at 4:40 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > $(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..944e68bd22b0 > --- /dev/null > +++ b/scripts/modules-check.sh > @@ -0,0 +1,18 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +# Warn if two or more modules have the same basename > +check_same_name_modules() > +{ > + same_name_modules=$(cat modules.order modules.builtin | \ > + xargs basename -a | sort | uniq -d) I noticed a bug here. allnoconfig + CONFIG_MODULES=y will create empty modules.order and modules.builtin. Then, 'basename -a' will emit the error messages since it receives zero arguments. basename: missing operand Try 'basename --help' for more information. I can fix it by checking the size of them. # If both modules.order and modules.builtin are empty, # "basename -a" emits error messages. if [ ! -s modules.order -a ! -s modules.builtin ]; then return fi same_name_modules=$(cat modules.order modules.builtin | \ xargs basename -a | sort | uniq -d) I wonder if there is a more elegant way... > + for m in $same_name_modules > + do > + echo "warning: same basename '$m' if the following are built as modules:" > + grep --no-filename -e /$m modules.order modules.builtin | \ > + sed 's:^kernel/: :' > + done > +} > + > +check_same_name_modules > -- > 2.17.1 > -- Best Regards Masahiro Yamada
On Thu, May 16, 2019 at 03:07:54AM +0900, Masahiro Yamada wrote: > On Wed, May 15, 2019 at 4:40 PM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > $(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..944e68bd22b0 > > --- /dev/null > > +++ b/scripts/modules-check.sh > > @@ -0,0 +1,18 @@ > > +#!/bin/sh > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +# Warn if two or more modules have the same basename > > +check_same_name_modules() > > +{ > > + same_name_modules=$(cat modules.order modules.builtin | \ > > + xargs basename -a | sort | uniq -d) > > > I noticed a bug here. > > > allnoconfig + CONFIG_MODULES=y > will create empty modules.order and modules.builtin. > > Then, 'basename -a' will emit the error messages > since it receives zero arguments. You can skip running it by adding "-r" to xargs: -r, --no-run-if-empty If the standard input does not contain any nonblanks, do not run the command. Normally, the command is run once even if there is no input. This option is a GNU extension. -- Kees Cook
On Thu, May 16, 2019 at 02:55:02AM +0900, Masahiro Yamada wrote: > > On Thu, May 16, 2019 at 1:20 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, May 15, 2019 at 04:53:15PM +0900, Masahiro Yamada wrote: > > > On Wed, May 15, 2019 at 4:40 PM 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..944e68bd22b0 > > > > --- /dev/null > > > > +++ b/scripts/modules-check.sh > > > > @@ -0,0 +1,18 @@ > > > > +#!/bin/sh > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +# Warn if two or more modules have the same basename > > > > +check_same_name_modules() > > > > +{ > > > > + same_name_modules=$(cat modules.order modules.builtin | \ > > > > + xargs basename -a | sort | uniq -d) > > > > While probably it'll never be a problem, just for robustness, I'd add "--" > > to the end basename to terminate argument interpretation: > > > > xargs basename -a -- | sort | ... > > > Sorry for my ignorance, but could you > teach me the effect of "--" ? > > > I sometimes use "--" as a separator > when there is ambiguity in arguments > for example, "git log <revision> -- <path>" > > > In this case, what is intended by "--"? It means "end of arguments" so that whatever xargs passes into the program aren't interpretted as an argument. In this case, if there was a module path somehow ever named --weird/build/path/foo.o, xargs would launch basename as: basename -a --weird/build/path/foo.o and basename would fail since it didn't recognize the argument. Having "--" will stop argument parsing: basename -a -- --weird/build/path/foo.o This is just a robustness suggestion that I always recommend for xargs piping, since this can turn into a security flaw (though not here) when an argument may have behavioral side-effects. So, it's just a thing that always jumps out at me, though in this particular case I don't think we could ever see it cause a problem, but better to always write these xargs patterns as safely as possible. -- Kees Cook
From: Masahiro Yamada > Sent: 15 May 2019 18:55 ... > > xargs basename -a -- | sort | ... > > Sorry for my ignorance, but could you > teach me the effect of "--" ? > > I sometimes use "--" as a separator > when there is ambiguity in arguments > for example, "git log <revision> -- <path>" > > In this case, what is intended by "--"? The '--' stops getopt() from parsing any more parameters. Useful things like 'grep -- -q' which will search for the string '-q' rather than treating it as a command line option. This is all made more horrid by a decision by the writers of glibc getopt() to 'permute' argv[] so that 'options' can follow 'nonoptions' ie it converts: prog file -arg to prog -arg file The only program the historically allowed 'late' options was 'rlogin hostname -l username'. This is just broken..... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On Thu, May 16, 2019 at 6:00 PM David Laight <David.Laight@aculab.com> wrote: > > From: Masahiro Yamada > > Sent: 15 May 2019 18:55 > ... > > > xargs basename -a -- | sort | ... > > > > Sorry for my ignorance, but could you > > teach me the effect of "--" ? > > > > I sometimes use "--" as a separator > > when there is ambiguity in arguments > > for example, "git log <revision> -- <path>" > > > > In this case, what is intended by "--"? > > The '--' stops getopt() from parsing any more parameters. > Useful things like 'grep -- -q' which will search for the > string '-q' rather than treating it as a command line option. > > This is all made more horrid by a decision by the writers > of glibc getopt() to 'permute' argv[] so that 'options' > can follow 'nonoptions' ie it converts: > prog file -arg > to > prog -arg file > The only program the historically allowed 'late' options > was 'rlogin hostname -l username'. > This is just broken..... Ah, I see. This does not happen for modules.order or modules.builtin, but I will use '--' just in case. Thanks. > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) -- Best Regards Masahiro Yamada
Hi Kees, On Thu, May 16, 2019 at 3:31 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, May 16, 2019 at 03:07:54AM +0900, Masahiro Yamada wrote: > > On Wed, May 15, 2019 at 4:40 PM Masahiro Yamada > > <yamada.masahiro@socionext.com> wrote: > > > > > $(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..944e68bd22b0 > > > --- /dev/null > > > +++ b/scripts/modules-check.sh > > > @@ -0,0 +1,18 @@ > > > +#!/bin/sh > > > +# SPDX-License-Identifier: GPL-2.0 > > > + > > > +# Warn if two or more modules have the same basename > > > +check_same_name_modules() > > > +{ > > > + same_name_modules=$(cat modules.order modules.builtin | \ > > > + xargs basename -a | sort | uniq -d) > > > > > > I noticed a bug here. > > > > > > allnoconfig + CONFIG_MODULES=y > > will create empty modules.order and modules.builtin. > > > > Then, 'basename -a' will emit the error messages > > since it receives zero arguments. > > You can skip running it by adding "-r" to xargs: > > -r, --no-run-if-empty > If the standard input does not contain any nonblanks, do not run > the command. Normally, the command is run once even if there is > no input. This option is a GNU extension. Good idea! I will use this in v2. Thanks. -- Best Regards Masahiro Yamada
Hi Kees, On Thu, May 16, 2019 at 3:38 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, May 16, 2019 at 02:55:02AM +0900, Masahiro Yamada wrote: > > > > On Thu, May 16, 2019 at 1:20 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Wed, May 15, 2019 at 04:53:15PM +0900, Masahiro Yamada wrote: > > > > On Wed, May 15, 2019 at 4:40 PM 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..944e68bd22b0 > > > > > --- /dev/null > > > > > +++ b/scripts/modules-check.sh > > > > > @@ -0,0 +1,18 @@ > > > > > +#!/bin/sh > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > + > > > > > +# Warn if two or more modules have the same basename > > > > > +check_same_name_modules() > > > > > +{ > > > > > + same_name_modules=$(cat modules.order modules.builtin | \ > > > > > + xargs basename -a | sort | uniq -d) > > > > > > While probably it'll never be a problem, just for robustness, I'd add "--" > > > to the end basename to terminate argument interpretation: > > > > > > xargs basename -a -- | sort | ... > > > > > > Sorry for my ignorance, but could you > > teach me the effect of "--" ? > > > > > > I sometimes use "--" as a separator > > when there is ambiguity in arguments > > for example, "git log <revision> -- <path>" > > > > > > In this case, what is intended by "--"? > > It means "end of arguments" so that whatever xargs passes into the > program aren't interpretted as an argument. In this case, if there was > a module path somehow ever named --weird/build/path/foo.o, xargs would > launch basename as: > > basename -a --weird/build/path/foo.o > > and basename would fail since it didn't recognize the argument. Having > "--" will stop argument parsing: > > basename -a -- --weird/build/path/foo.o > > This is just a robustness suggestion that I always recommend for xargs > piping, since this can turn into a security flaw (though not here) when > an argument may have behavioral side-effects. So, it's just a thing that > always jumps out at me, though in this particular case I don't think > we could ever see it cause a problem, but better to always write these > xargs patterns as safely as possible. I did not think about the security issue. Thanks for your expert comments! -- Best Regards Masahiro Yamada
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..944e68bd22b0 --- /dev/null +++ b/scripts/modules-check.sh @@ -0,0 +1,18 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +# Warn if two or more modules have the same basename +check_same_name_modules() +{ + same_name_modules=$(cat modules.order modules.builtin | \ + xargs basename -a | sort | uniq -d) + + for m in $same_name_modules + do + echo "warning: same basename '$m' if the following are built as modules:" + grep --no-filename -e /$m modules.order modules.builtin | \ + sed 's:^kernel/: :' + done +} + +check_same_name_modules
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 occasionally shows up with obscure error message when this kind of race occurs. It is not trivial to catch this potential issue by eyes. Hence, this script. I compile-tested allmodconfig for the latest kernel as of writing, it detected the following: warning: same basename '88pm800.ko' if the following are built as modules: drivers/regulator/88pm800.ko drivers/mfd/88pm800.ko warning: same basename 'adv7511.ko' if the following are built as modules: drivers/gpu/drm/bridge/adv7511/adv7511.ko drivers/media/i2c/adv7511.ko warning: same basename 'asix.ko' if the following are built as modules: drivers/net/phy/asix.ko drivers/net/usb/asix.ko warning: same basename 'coda.ko' if the following are built as modules: fs/coda/coda.ko drivers/media/platform/coda/coda.ko warning: same basename 'realtek.ko' 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> --- [Alternative fix ? ] I do not know about the user experience of modprobe etc. when two different modules have the same name. It does not matter if this is correctly handled by modules.order? If this is just a problem of the build system, it is pretty easy to fix. For example, if we prepend the directory path, parallel build will never write to the same file simultanously. asix.mod -> drivers/net/phy/asix.mod asix.mod -> drivers/net/usb/asix.mod [Futher discussion] Linus Torvalds pointed out that it is silly to add the same prefix to each file since the sub-system is already represented by the directory path. See this: https://lkml.org/lkml/2017/7/12/430 We can keep the basename short enough to distinguish in the subsytem in theory. So, I am not surprised to see the same file name in different directory locations. On the other hand, a module is named after the file name when it consists of a single C source file. Of course, you can always give a different module name. For example, see drivers/nvmem/Makefile I am not a big fan of it since it looks ugly. I think we can play it by ear, but I just wanted to point out this related to the module name uniqueness. Makefile | 1 + scripts/modules-check.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100755 scripts/modules-check.sh -- 2.17.1