diff mbox series

[v2] kbuild: check MODULE_* macros in non-modular code

Message ID 20191122081100.27695-1-yamada.masahiro@socionext.com
State New
Headers show
Series [v2] kbuild: check MODULE_* macros in non-modular code | expand

Commit Message

Masahiro Yamada Nov. 22, 2019, 8:11 a.m. UTC 
Paul Gortmaker sent a lot of patches to remove orphan modular code.
You can see his contributions by:

  $ git log --grep='make.* explicitly non-modular'

To help this work, this commit adds simple shell-script to detect
MODULE_ tags used in non-modular code.

It displays suspicious use of MODULE_LICENSE, MODULE_AUTHOR,
MODULE_DESCRIPTION, etc.

I was not sure about module_param() or MODULE_PARM_DESC(). A lot of
non-modular code uses module_param() to prefix the kernel parameter
with the file name it resides in. If we changed module_param() to
core_param(), the interface would be broken. MODULE_PARM_DESC() in
non-modular code could be turned into comments or something, but I
am not sure. I did not check them.

I built x86_64_defconfig of v5.4-rc8, and this script detected
the following:

notice: asymmetric_keys: MODULE macros found in non-modular code
notice: binfmt_elf: MODULE macros found in non-modular code
notice: bsg: MODULE macros found in non-modular code
notice: compat_binfmt_elf: MODULE macros found in non-modular code
notice: component: MODULE macros found in non-modular code
notice: debugfs: MODULE macros found in non-modular code
notice: drm_mipi_dsi: MODULE macros found in non-modular code
notice: freq_table: MODULE macros found in non-modular code
notice: glob: MODULE macros found in non-modular code
notice: intel_pstate: MODULE macros found in non-modular code
notice: n_null: MODULE macros found in non-modular code
notice: nvmem_core: MODULE macros found in non-modular code
notice: power_supply: MODULE macros found in non-modular code
notice: thermal_sys: MODULE macros found in non-modular code
notice: tracefs: MODULE macros found in non-modular code
notice: vgacon: MODULE macros found in non-modular code
 To fix above, check MODULE_LICENSE(), MODULE_AUTHOR(), etc.
 Please check #include <linux/module.h>, THIS_MODULE, too.

I confirmed they are all valid.

Maybe the 'debugfs' is unclear because there are tons of debugfs
stuff in the source tree. It is talking about MODULE_ALIAS_FS()
in fs/debugfs/inode.c because fs/debugfs/debugfs.o never becomes
a module.

[How to fix the warnings]

Let's take 'asymmetric_keys' as an example.

(1) grep Makefiles to find the relevant code

$ git grep -A2 asymmetric_keys -- '*/Makefile' '*/Kbuild'
crypto/Makefile:obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys/
crypto/Makefile-obj-$(CONFIG_CRYPTO_HASH_INFO) += hash_info.o
crypto/Makefile-crypto_simd-y := simd.o
--
crypto/asymmetric_keys/Makefile:obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o
crypto/asymmetric_keys/Makefile-
crypto/asymmetric_keys/Makefile:asymmetric_keys-y := \
crypto/asymmetric_keys/Makefile-        asymmetric_type.o \
crypto/asymmetric_keys/Makefile-        restrict.o \

Then, you notice it is associated with CONFIG_ASYMMETRIC_KEY_TYPE
and is a composite object that consists of asymmetric_type.o,
restrict.o, ...

(2) Confirm the CONFIG is boolean

$ git grep -A2 'config ASYMMETRIC_KEY_TYPE' -- '*/Kconfig*'
crypto/asymmetric_keys/Kconfig:menuconfig ASYMMETRIC_KEY_TYPE
crypto/asymmetric_keys/Kconfig- bool "Asymmetric (public-key cryptographic) key type"
crypto/asymmetric_keys/Kconfig- depends on KEYS

Now you are sure it never get compiled as a module since
ASYMMETRIC_KEY_TYPE is a bool type option.

(3) Grep the source file(s)

$ grep '^MODULE' crypto/asymmetric_keys/asymmetric_type.c
MODULE_LICENSE("GPL");

Remove the orphan MODULE tags. You may also need to do some additional
works such as:

 - replace module_*_driver with builtin_*_driver
 - replace <linux/module.h> with <linux/init.h>
 - remove module_exit code
 - move credit in MODULE_AUTHOR() to the top of the file

Please see Paul's commits.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

Changes in v2:
  - Remove redundant back-slashes after the pipe operator '|'

 scripts/modules-check.sh | 54 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

-- 
2.17.1

Comments

Kees Cook Nov. 22, 2019, 5:39 p.m. UTC | #1 
On Fri, Nov 22, 2019 at 05:11:00PM +0900, Masahiro Yamada wrote:
> Paul Gortmaker sent a lot of patches to remove orphan modular code.

> You can see his contributions by:

> 

>   $ git log --grep='make.* explicitly non-modular'

> 

> To help this work, this commit adds simple shell-script to detect

> MODULE_ tags used in non-modular code.

> 

> It displays suspicious use of MODULE_LICENSE, MODULE_AUTHOR,

> MODULE_DESCRIPTION, etc.

> 

> I was not sure about module_param() or MODULE_PARM_DESC(). A lot of

> non-modular code uses module_param() to prefix the kernel parameter

> with the file name it resides in. If we changed module_param() to

> core_param(), the interface would be broken. MODULE_PARM_DESC() in

> non-modular code could be turned into comments or something, but I

> am not sure. I did not check them.


Right -- this is a very useful mechanism to get "namespace scoped"
boot params and runtime tunables. If we want to separate that from
module code, that's fine, but I'd agree that we should not move users to
core_param().

> 

> I built x86_64_defconfig of v5.4-rc8, and this script detected

> the following:

> 

> [...]

> notice: binfmt_elf: MODULE macros found in non-modular code

> [...]

> notice: compat_binfmt_elf: MODULE macros found in non-modular code


IIUC, looking at binfmt_elf, the fix is the removal of:

- function exit_elf_binfmt()
- use of module_exit()
- use of MODULE_LICENSE

however, module.h needs to remain because of THIS_MODULE usage in
struct linux_binfmt (other binfmts may be modular).

This that a correct evaluation?

> [...]

>  To fix above, check MODULE_LICENSE(), MODULE_AUTHOR(), etc.

>  Please check #include <linux/module.h>, THIS_MODULE, too.

> 

> I confirmed they are all valid.

> 

> Maybe the 'debugfs' is unclear because there are tons of debugfs

> stuff in the source tree. It is talking about MODULE_ALIAS_FS()

> in fs/debugfs/inode.c because fs/debugfs/debugfs.o never becomes

> a module.

> [...]

> 

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


This patch looks good but lets clean up what it detects first so we
don't make the build noisy...

Reviewed-by: Kees Cook <keescook@chromium.org>


-Kees

> ---

> 

> Changes in v2:

>   - Remove redundant back-slashes after the pipe operator '|'

> 

>  scripts/modules-check.sh | 54 ++++++++++++++++++++++++++++++++++++++++

>  1 file changed, 54 insertions(+)

> 

> diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh

> index f51f446707b8..7975aa61ddb8 100755

> --- a/scripts/modules-check.sh

> +++ b/scripts/modules-check.sh

> @@ -13,4 +13,58 @@ check_same_name_modules()

>  	done

>  }

>  

> +# Check MODULE_ macros in non-modular code

> +check_orphan_module_macros()

> +{

> +	# modules.builtin.modinfo is created while linking vmlinux.

> +	# It may not exist when you do 'make modules'.

> +	if [ ! -r modules.builtin.modinfo ]; then

> +		return

> +	fi

> +

> +	# modules.builtin lists *real* built-in modules, i.e. controlled by

> +	# tristate CONFIG options, but currently built with =y.

> +	#

> +	# modules.builtin.modinfo is the list of MODULE_ macros compiled

> +	# into vmlinux.

> +	#

> +	# By diff'ing them, users of bogus MODULE_* macros will show up.

> +

> +	# Kbuild replaces ',' and '-' in file names with '_' for use in C.

> +	real_builtin_modules=$(sed -e 's:.*/::' -e 's/\.ko$//' -e 's/,/_/g' \

> +			       -e 's/-/_/g' modules.builtin | sort | uniq)

> +

> +	show_hint=

> +

> +	# Exclude '.paramtype=' and '.param=' to skip checking module_param()

> +	# and MODULE_PARM_DESC().

> +	module_macro_users=$(tr '\0' '\n' < modules.builtin.modinfo |

> +			     sed -e '/\.parmtype=/d' -e '/\.parm=/d' |

> +			     sed -n 's/\..*//p' | sort | uniq)

> +

> +	for m in $module_macro_users

> +	do

> +		warn=1

> +

> +		for n in $real_builtin_modules

> +		do

> +			if [ "$m" = "$n" ]; then

> +				warn=

> +				break

> +			fi

> +		done

> +

> +		if [ -n "$warn" ]; then

> +			echo "notice: $m: MODULE macros found in non-modular code"

> +			show_hint=1

> +		fi

> +	done

> +

> +	if [ -n "$show_hint" ]; then

> +		echo " To fix above, check MODULE_LICENSE(), MODULE_AUTHOR(), etc."

> +		echo " Please check #include <linux/module.h>, THIS_MODULE, too."

> +	fi

> +}

> +

>  check_same_name_modules

> +check_orphan_module_macros

> -- 

> 2.17.1

> 


-- 
Kees Cook
diff mbox series

Patch

diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh
index f51f446707b8..7975aa61ddb8 100755
--- a/scripts/modules-check.sh
+++ b/scripts/modules-check.sh
@@ -13,4 +13,58 @@  check_same_name_modules()
 	done
 }
 
+# Check MODULE_ macros in non-modular code
+check_orphan_module_macros()
+{
+	# modules.builtin.modinfo is created while linking vmlinux.
+	# It may not exist when you do 'make modules'.
+	if [ ! -r modules.builtin.modinfo ]; then
+		return
+	fi
+
+	# modules.builtin lists *real* built-in modules, i.e. controlled by
+	# tristate CONFIG options, but currently built with =y.
+	#
+	# modules.builtin.modinfo is the list of MODULE_ macros compiled
+	# into vmlinux.
+	#
+	# By diff'ing them, users of bogus MODULE_* macros will show up.
+
+	# Kbuild replaces ',' and '-' in file names with '_' for use in C.
+	real_builtin_modules=$(sed -e 's:.*/::' -e 's/\.ko$//' -e 's/,/_/g' \
+			       -e 's/-/_/g' modules.builtin | sort | uniq)
+
+	show_hint=
+
+	# Exclude '.paramtype=' and '.param=' to skip checking module_param()
+	# and MODULE_PARM_DESC().
+	module_macro_users=$(tr '\0' '\n' < modules.builtin.modinfo |
+			     sed -e '/\.parmtype=/d' -e '/\.parm=/d' |
+			     sed -n 's/\..*//p' | sort | uniq)
+
+	for m in $module_macro_users
+	do
+		warn=1
+
+		for n in $real_builtin_modules
+		do
+			if [ "$m" = "$n" ]; then
+				warn=
+				break
+			fi
+		done
+
+		if [ -n "$warn" ]; then
+			echo "notice: $m: MODULE macros found in non-modular code"
+			show_hint=1
+		fi
+	done
+
+	if [ -n "$show_hint" ]; then
+		echo " To fix above, check MODULE_LICENSE(), MODULE_AUTHOR(), etc."
+		echo " Please check #include <linux/module.h>, THIS_MODULE, too."
+	fi
+}
+
 check_same_name_modules
+check_orphan_module_macros