Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step

[Masahiro Yamada]

On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@xxxxxxxxx> wrote:
>
> Currently Makefile.modinst does three tasks on each module built:
> - Install modules
> - Sign modules
> - Compress modules
>
> All the above tasks happen from a single place.
>
> This patch divides this task further and uses a different makefile for
> each task.
> Signing module logic is completely refactored and everything happens
> from a shell script now.
>
> Signed-off-by: Shreenidhi Shedi <yesshedi@xxxxxxxxx>


This patch is bad in multiple ways.

1. Break "make modules_sign"
2.   Serialize the installation steps, that is, works less efficiently
3.   Increase code without adding any benefits.


There is no good reason to do these changes.

NACK.





> ---
>  scripts/Makefile.compress |  53 ++++++++++++++++++
>  scripts/Makefile.install  |  66 +++++++++++++++++++++++
>  scripts/Makefile.modinst  | 111 +++-----------------------------------
>  scripts/Makefile.sign     |  37 +++++++++++++
>  scripts/signfile.sh       |  24 +++++++++
>  5 files changed, 186 insertions(+), 105 deletions(-)
>  create mode 100644 scripts/Makefile.compress
>  create mode 100644 scripts/Makefile.install
>  create mode 100644 scripts/Makefile.sign
>  create mode 100755 scripts/signfile.sh
>
> diff --git a/scripts/Makefile.compress b/scripts/Makefile.compress
> new file mode 100644
> index 000000000000..35d337ac9b6c
> --- /dev/null
> +++ b/scripts/Makefile.compress
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Compressing modules
> +# ==========================================================================
> +
> +PHONY := __modcompress
> +__modcompress:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +modules := $(call read-file, $(MODORDER))
> +
> +ifeq ($(KBUILD_EXTMOD),)
> +dst := $(MODLIB)/kernel
> +else
> +INSTALL_MOD_DIR ?= updates
> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> +endif
> +
> +suffix-y                               :=
> +suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
> +suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
> +suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
> +
> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> +
> +__modcompress: $(modules)
> +       @:
> +
> +#
> +# Compression
> +#
> +quiet_cmd_gzip = GZIP    $@
> +      cmd_gzip = $(KGZIP) -n -f $<
> +quiet_cmd_xz = XZ      $@
> +      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
> +quiet_cmd_zstd = ZSTD    $@
> +      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
> +
> +$(dst)/%.ko.gz: $(dst)/%.ko FORCE
> +       $(call cmd,gzip)
> +
> +$(dst)/%.ko.xz: $(dst)/%.ko FORCE
> +       $(call cmd,xz)
> +
> +$(dst)/%.ko.zst: $(dst)/%.ko FORCE
> +       $(call cmd,zstd)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/Makefile.install b/scripts/Makefile.install
> new file mode 100644
> index 000000000000..40c496cb99dc
> --- /dev/null
> +++ b/scripts/Makefile.install
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Installing modules
> +# ==========================================================================
> +
> +PHONY := __modinstall
> +__modinstall:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +modules := $(call read-file, $(MODORDER))
> +
> +ifeq ($(KBUILD_EXTMOD),)
> +dst := $(MODLIB)/kernel
> +else
> +INSTALL_MOD_DIR ?= updates
> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> +endif
> +
> +$(foreach x, % :, $(if $(findstring $x, $(dst)), \
> +       $(error module installation path cannot contain '$x')))
> +
> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> +
> +__modinstall: $(modules)
> +       @:
> +
> +#
> +# Installation
> +#
> +quiet_cmd_install = INSTALL $@
> +      cmd_install = mkdir -p $(dir $@); cp $< $@
> +
> +# Strip
> +#
> +# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
> +# are installed. If INSTALL_MOD_STRIP is '1', then the default option
> +# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
> +# as the options to the strip command.
> +ifdef INSTALL_MOD_STRIP
> +
> +ifeq ($(INSTALL_MOD_STRIP),1)
> +strip-option := --strip-debug
> +else
> +strip-option := $(INSTALL_MOD_STRIP)
> +endif
> +
> +quiet_cmd_strip = STRIP   $@
> +      cmd_strip = $(STRIP) $(strip-option) $@
> +
> +else
> +
> +quiet_cmd_strip =
> +      cmd_strip = :
> +
> +endif
> +
> +$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
> +       $(call cmd,install)
> +       $(call cmd,strip)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
> index ab0c5bd1a60f..d87e09e57963 100644
> --- a/scripts/Makefile.modinst
> +++ b/scripts/Makefile.modinst
> @@ -1,116 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # ==========================================================================
> -# Installing modules
> +# Install, Sign & Compress modules
>  # ==========================================================================
>
> -PHONY := __modinst
> -__modinst:
> -
>  include include/config/auto.conf
>  include $(srctree)/scripts/Kbuild.include
>
> -modules := $(call read-file, $(MODORDER))
> -
> -ifeq ($(KBUILD_EXTMOD),)
> -dst := $(MODLIB)/kernel
> -else
> -INSTALL_MOD_DIR ?= updates
> -dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> -endif
> -
> -$(foreach x, % :, $(if $(findstring $x, $(dst)), \
> -       $(error module installation path cannot contain '$x')))
> -
> -suffix-y                               :=
> -suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
> -suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
> -suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
> -
> -modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> -
> -__modinst: $(modules)
> -       @:
> -
> -#
> -# Installation
> -#
> -quiet_cmd_install = INSTALL $@
> -      cmd_install = mkdir -p $(dir $@); cp $< $@
> -
> -# Strip
> -#
> -# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
> -# are installed. If INSTALL_MOD_STRIP is '1', then the default option
> -# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
> -# as the options to the strip command.
> -ifdef INSTALL_MOD_STRIP
> -
> -ifeq ($(INSTALL_MOD_STRIP),1)
> -strip-option := --strip-debug
> -else
> -strip-option := $(INSTALL_MOD_STRIP)
> -endif
> -
> -quiet_cmd_strip = STRIP   $@
> -      cmd_strip = $(STRIP) $(strip-option) $@
> -
> -else
> -
> -quiet_cmd_strip =
> -      cmd_strip = :
> -
> -endif
> -
> -#
> -# Signing
> -# Don't stop modules_install even if we can't sign external modules.
> -#
> -ifeq ($(CONFIG_MODULE_SIG_ALL),y)
> -ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
> -sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
> -else
> -sig-key := $(CONFIG_MODULE_SIG_KEY)
> -endif
> -quiet_cmd_sign = SIGN    $@
> -      cmd_sign = scripts/sign-file $(CONFIG_MODULE_SIG_HASH) "$(sig-key)" certs/signing_key.x509 $@ \
> -                 $(if $(KBUILD_EXTMOD),|| true)
> -else
> -quiet_cmd_sign :=
> -      cmd_sign := :
> -endif
> -
> -ifeq ($(modules_sign_only),)
> -
> -$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
> -       $(call cmd,install)
> -       $(call cmd,strip)
> -       $(call cmd,sign)
> -
> -else
> -
> -$(dst)/%.ko: FORCE
> -       $(call cmd,sign)
> -
> -endif
> -
> -#
> -# Compression
> -#
> -quiet_cmd_gzip = GZIP    $@
> -      cmd_gzip = $(KGZIP) -n -f $<
> -quiet_cmd_xz = XZ      $@
> -      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
> -quiet_cmd_zstd = ZSTD    $@
> -      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
> -
> -$(dst)/%.ko.gz: $(dst)/%.ko FORCE
> -       $(call cmd,gzip)
> -
> -$(dst)/%.ko.xz: $(dst)/%.ko FORCE
> -       $(call cmd,xz)
> +PHONY := __modinst
>
> -$(dst)/%.ko.zst: $(dst)/%.ko FORCE
> -       $(call cmd,zstd)
> +__modinst: FORCE
> +       $(MAKE) -f scripts/Makefile.install
> +       $(MAKE) -f scripts/Makefile.sign
> +       $(MAKE) -f scripts/Makefile.compress
>
>  PHONY += FORCE
>  FORCE:
> diff --git a/scripts/Makefile.sign b/scripts/Makefile.sign
> new file mode 100644
> index 000000000000..d6b242b16657
> --- /dev/null
> +++ b/scripts/Makefile.sign
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Signing modules
> +# ==========================================================================
> +
> +PHONY := __modsign
> +__modsign:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +#
> +# Signing
> +# Don't stop modules_install even if we can't sign external modules.
> +#
> +ifeq ($(CONFIG_MODULE_SIG_ALL),y)
> +ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
> +sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
> +else
> +sig-key := $(CONFIG_MODULE_SIG_KEY)
> +endif
> +quiet_cmd_sign = SIGNING ALL MODULES ...
> +      cmd_sign = $(CONFIG_SHELL) $(srctree)/scripts/signfile.sh \
> +                                        "$(CONFIG_MODULE_SIG_HASH)" \
> +                                        "$(sig-key)"
> +else
> +quiet_cmd_sign :=
> +      cmd_sign := :
> +endif
> +
> +__modsign: FORCE
> +       $(call cmd,sign)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/signfile.sh b/scripts/signfile.sh
> new file mode 100755
> index 000000000000..b2b58bfbd5ba
> --- /dev/null
> +++ b/scripts/signfile.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# A sign-file wrapper used by scripts/Makefile.sign
> +
> +#set -x
> +
> +if test $# -ne 2; then
> +       echo "Usage: $0 <hash-algo> <sign-key>" >&2
> +       exit 1
> +fi
> +
> +SIG_HASH="$1"
> +SIG_KEY="$2"
> +
> +MODULES_PATH="${INSTALL_MOD_PATH}/lib/modules/${KERNELRELEASE}"
> +
> +find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> +       xargs -r -0 -P$(nproc) -x -n32 sh -c "\
> +${srctree}/scripts/sign-file \
> +-a \"${SIG_HASH}\" \
> +-i \"${SIG_KEY}\" \
> +-x ${srctree}/certs/signing_key.x509 \
> +-b \$@ \$0"
> --
> 2.41.0
>


-- 
Best Regards
Masahiro Yamada