[RFC PATCH] kbuild: check uniqueness of basename of modules

[RFC PATCH] kbuild: check uniqueness of basename of modules

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

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Masahiro Yamada]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Kees Cook]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Masahiro Yamada]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[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

RE: [RFC PATCH] kbuild: check uniqueness of basename of modules

[David Laight]

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)

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Masahiro Yamada]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Arnd Bergmann]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Greg KH]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Masahiro Yamada]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Greg KH]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Masahiro Yamada]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[Kees Cook]

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

Re: [RFC PATCH] kbuild: check uniqueness of basename of modules

[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