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

2019-05-17 Thread Masahiro Yamada
On Fri, May 17, 2019 at 2:34 PM Stephen Rothwell  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 
>  wrote:
> >
>
> > diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh
> > new file mode 100755
> > index ..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 
>
> --
> Cheers,
> Stephen Rothwell



-- 
Best Regards
Masahiro Yamada


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

2019-05-17 Thread Masahiro Yamada
On Fri, May 17, 2019 at 2:46 PM Lucas De Marchi
 wrote:
>
> On Thu, May 16, 2019 at 10:37 PM Greg KH  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
> > >  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 
> > > > Signed-off-by: Masahiro Yamada 
> > > > Reviewed-by: Kees Cook 
> > > > ---
> > >
> > >
> > > 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 De Marchi



-- 
Best Regards
Masahiro Yamada


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

2019-05-17 Thread Masahiro Yamada
On Fri, May 17, 2019 at 6:25 PM Alexander Kapshuk
 wrote:
>
> On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch
>  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  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

2019-05-17 Thread Bernd Petrovitsch
On 17/05/2019 11:25, Alexander Kapshuk wrote:
> On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch
>  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  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


pEpkey.asc
Description: application/pgp-keys


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

2019-05-17 Thread 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 : be...@petrovitsch.priv.at
 LUGA : http://www.luga.at


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

2019-05-17 Thread Alexander Kapshuk
On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch
 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  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 : be...@petrovitsch.priv.at
>  LUGA : http://www.luga.at


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

2019-05-17 Thread Alexander Kapshuk
On Fri, May 17, 2019 at 8:33 AM Masahiro Yamada
 wrote:
>
> In the recent build test of linux-next, Stephen saw a build error
> caused by a broken .tmp_versions/*.mod file:
>
>   https://lkml.org/lkml/2019/5/13/991
>
> drivers/net/phy/asix.ko and drivers/net/usb/asix.ko have the same
> basename, and there is a race in generating .tmp_versions/asix.mod
>
> Kbuild has not checked this before, and it suddenly shows up with
> obscure error message when this kind of race occurs.
>
> Non-unique module names cause various sort of problems, but it is
> not trivial to catch them by eyes.
>
> Hence, this script.
>
> It checks not only real modules, but also built-in modules (i.e.
> controlled by tristate CONFIG option, but currently compiled with =y).
> Non-unique names for built-in modules also cause problems because
> /sys/modules/ would fall over.
>
> I tested allmodconfig on the latest kernel, and it detected the
> following:
>
> warning: same basename if the following are built as modules:
>   drivers/regulator/88pm800.ko
>   drivers/mfd/88pm800.ko
> warning: same basename if the following are built as modules:
>   drivers/gpu/drm/bridge/adv7511/adv7511.ko
>   drivers/media/i2c/adv7511.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/asix.ko
>   drivers/net/usb/asix.ko
> warning: same basename if the following are built as modules:
>   fs/coda/coda.ko
>   drivers/media/platform/coda/coda.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/realtek.ko
>   drivers/net/dsa/realtek.ko
>
> Reported-by: Stephen Rothwell 
> Signed-off-by: Masahiro Yamada 
> Reviewed-by: Kees Cook 
> ---
>
> 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 ..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

2019-05-16 Thread Lucas De Marchi
On Thu, May 16, 2019 at 10:37 PM Greg KH  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
> >  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 
> > > Signed-off-by: Masahiro Yamada 
> > > Reviewed-by: Kees Cook 
> > > ---
> >
> >
> > 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 De Marchi


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

2019-05-16 Thread 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
>  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 
> > Signed-off-by: Masahiro Yamada 
> > Reviewed-by: Kees Cook 
> > ---
> 
> 
> 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

2019-05-16 Thread Stephen Rothwell
Hi Masahiro,

Thanks for this, looks good to me.  Just a nit below.

On Fri, 17 May 2019 13:27:53 +0900 Masahiro Yamada 
 wrote:
>

> diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh
> new file mode 100755
> index ..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 

-- 
Cheers,
Stephen Rothwell


pgpO_BjA7Nx9n.pgp
Description: OpenPGP digital signature


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

2019-05-16 Thread Masahiro Yamada
On Fri, May 17, 2019 at 1:29 PM Masahiro Yamada
 wrote:
>
> In the recent build test of linux-next, Stephen saw a build error
> caused by a broken .tmp_versions/*.mod file:
>
>   https://lkml.org/lkml/2019/5/13/991
>
> drivers/net/phy/asix.ko and drivers/net/usb/asix.ko have the same
> basename, and there is a race in generating .tmp_versions/asix.mod
>
> Kbuild has not checked this before, and it suddenly shows up with
> obscure error message when this kind of race occurs.
>
> Non-unique module names cause various sort of problems, but it is
> not trivial to catch them by eyes.
>
> Hence, this script.
>
> It checks not only real modules, but also built-in modules (i.e.
> controlled by tristate CONFIG option, but currently compiled with =y).
> Non-unique names for built-in modules also cause problems because
> /sys/modules/ would fall over.
>
> I tested allmodconfig on the latest kernel, and it detected the
> following:
>
> warning: same basename if the following are built as modules:
>   drivers/regulator/88pm800.ko
>   drivers/mfd/88pm800.ko
> warning: same basename if the following are built as modules:
>   drivers/gpu/drm/bridge/adv7511/adv7511.ko
>   drivers/media/i2c/adv7511.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/asix.ko
>   drivers/net/usb/asix.ko
> warning: same basename if the following are built as modules:
>   fs/coda/coda.ko
>   drivers/media/platform/coda/coda.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/realtek.ko
>   drivers/net/dsa/realtek.ko
>
> Reported-by: Stephen Rothwell 
> Signed-off-by: Masahiro Yamada 
> Reviewed-by: Kees Cook 
> ---


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


[PATCH v2] kbuild: check uniqueness of module names

2019-05-16 Thread 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 
Signed-off-by: Masahiro Yamada 
Reviewed-by: Kees Cook 
---

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