[PATCH v2] kbuild: check uniqueness of module names

[PATCH v2] kbuild: check uniqueness of module names

[Masahiro Yamada]

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

Re: [PATCH v2] kbuild: check uniqueness of module names

[Masahiro Yamada]

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...



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: check uniqueness of module names

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

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>

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] kbuild: check uniqueness of module names

[Greg KH]

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

Re: [PATCH v2] kbuild: check uniqueness of module names

[Lucas De Marchi]

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

Re: [PATCH v2] kbuild: check uniqueness of module names

[Alexander Kapshuk]

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.

Re: [PATCH v2] kbuild: check uniqueness of module names

[Bernd Petrovitsch]

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
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH v2] kbuild: check uniqueness of module names

[Alexander Kapshuk]

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

Re: [PATCH v2] kbuild: check uniqueness of module names

[Bernd Petrovitsch]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

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

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 2513 bytes --]

Re: [PATCH v2] kbuild: check uniqueness of module names

[Masahiro Yamada]

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.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: check uniqueness of module names

[Masahiro Yamada]

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



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: check uniqueness of module names

[Masahiro Yamada]

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



-- 
Best Regards
Masahiro Yamada