| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c98ffcdc-1e2f-4496-99a3-3b590002e5b1@gmail.com>
Date: Mon, 7 Aug 2023 13:38:30 +0530
From: Shreenidhi Shedi <yesshedi@...il.com>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: dhowells@...hat.com, dwmw2@...radead.org,
gregkh@...uxfoundation.org, nathan@...nel.org,
ndesaulniers@...gle.com, nicolas@...sle.eu,
linux-kernel@...r.kernel.org, sshedi@...are.com
Subject: Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
On 07/08/23 01:02, Masahiro Yamada wrote:
> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@...il.com> 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@...il.com>
>
>
> This patch is bad in multiple ways.
>
> 1. Break "make modules_sign"
Correct, somehow I missed it. I will fix it.
I'm using below command to test sign only option. Please let me know if
I should use something else.
make modules_sign modules_sign_only=1 INSTALL_MOD_PATH=$PWD/tmp -j8
> 2. Serialize the installation steps, that is, works less efficiently
Even in the existing system it happens in serially. And the existing
method takes more time than the proposed version.
root@...dev:~/linux-6.3.5 # ./test.sh orig
real 0m14.699s
user 0m55.519s
sys 0m9.036s
root@...dev:~/linux-6.3.5 # ./test.sh new
real 0m13.327s
user 0m46.885s
sys 0m6.770s
Here is my test script.
```
#!/bin/bash
set -e
if [ "$1" != "new" ] && [ "$1" != "orig" ]; then
echo "invalid arg, ($0 [orig|new])" >&2
exit 1
fi
rm -rf $PWD/tmp
s="scripts/sign-file.c"
m="scripts/Makefile.modinst"
fns=($s $m)
for f in ${fns[@]}; do
cp $f.$1 $f
done
cd scripts
gcc -o sign-file sign-file.c -lcrypto
cd -
time make modules_install INSTALL_MOD_PATH=$PWD/tmp -s -j$(nproc)
```
> 3. Increase code without adding any benefits.
>Agree with increased code but this change is one step closer to Unix
philosophy, do one thing well wrt modules_install.
>
> There is no good reason to do these chang >I hope the data I provided above to your 2nd point provides evidence
that this fix is improving existing system. Please take a look again.
> NACK.
Hi Masahiro Yamada,
Replies inline above.
Please correct me if my understanding is wrong. Thanks a lot for your
time and patience. Have a nice time ahead.
>
>
>
>
>
>> ---
>> 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
>>
>
>
--
Shedi
Powered by blists - more mailing lists