* [PATCH v7 1/8] sign-file: use getopt_long_only for parsing input args
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy Shreenidhi Shedi
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
- getopt_long_only gives an option to use long names for options, so
using it here to make the app usage easier.
- Use more easy to remember command line argument names
- Introduce cmd_opts structure to ease the handling of command line args
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 97 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 78 insertions(+), 19 deletions(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index 598ef5465f82..94228865b6cc 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -213,15 +213,77 @@ static X509 *read_x509(const char *x509_name)
return x509;
}
+struct cmd_opts {
+ char *raw_sig_name;
+ bool save_sig;
+ bool replace_orig;
+ bool raw_sig;
+ bool sign_only;
+#ifndef USE_PKCS7
+ unsigned int use_keyid;
+#endif
+};
+
+static void parse_args(int argc, char **argv, struct cmd_opts *opts)
+{
+ struct option cmd_options[] = {
+ {"rawsig", required_argument, 0, 's'},
+ {"savesig", no_argument, 0, 'p'},
+ {"signonly", no_argument, 0, 'd'},
+#ifndef USE_PKCS7
+ {"usekeyid", no_argument, 0, 'k'},
+#endif
+ {0, 0, 0, 0}
+ };
+
+ int opt;
+ int opt_index = 0;
+
+ do {
+#ifndef USE_PKCS7
+ opt = getopt_long_only(argc, argv, "pds:",
+ cmd_options, &opt_index);
+#else
+ opt = getopt_long_only(argc, argv, "pdks:",
+ cmd_options, &opt_index);
+#endif
+ switch (opt) {
+ case 's':
+ opts->raw_sig = true;
+ opts->raw_sig_name = optarg;
+ break;
+
+ case 'p':
+ opts->save_sig = true;
+ break;
+
+ case 'd':
+ opts->sign_only = true;
+ opts->save_sig = true;
+ break;
+
+#ifndef USE_PKCS7
+ case 'k':
+ opts->use_keyid = CMS_USE_KEYID;
+ break;
+#endif
+
+ case -1:
+ break;
+
+ default:
+ format();
+ break;
+ }
+ } while (opt != -1);
+}
+
int main(int argc, char **argv)
{
struct module_signature sig_info = { .id_type = PKEY_ID_PKCS7 };
char *hash_algo = NULL;
- char *private_key_name = NULL, *raw_sig_name = NULL;
+ char *private_key_name = NULL;
char *x509_name, *module_name, *dest_name;
- bool save_sig = false, replace_orig;
- bool sign_only = false;
- bool raw_sig = false;
unsigned char buf[4096];
unsigned long module_size, sig_size;
unsigned int use_signed_attrs;
@@ -229,13 +291,14 @@ int main(int argc, char **argv)
EVP_PKEY *private_key;
#ifndef USE_PKCS7
CMS_ContentInfo *cms = NULL;
- unsigned int use_keyid = 0;
#else
PKCS7 *pkcs7 = NULL;
#endif
X509 *x509;
BIO *bd, *bm;
- int opt, n;
+ int n;
+ struct cmd_opts opts = {};
+
OpenSSL_add_all_algorithms();
ERR_load_crypto_strings();
ERR_clear_error();
@@ -247,23 +310,19 @@ int main(int argc, char **argv)
#else
use_signed_attrs = PKCS7_NOATTR;
#endif
+ parse_args(argc, argv, &opts);
+ argc -= optind;
+ argv += optind;
- do {
- opt = getopt(argc, argv, "sdpk");
- switch (opt) {
- case 's': raw_sig = true; break;
- case 'p': save_sig = true; break;
- case 'd': sign_only = true; save_sig = true; break;
+ const char *raw_sig_name = opts.raw_sig_name;
+ const bool save_sig = opts.save_sig;
+ const bool raw_sig = opts.raw_sig;
+ const bool sign_only = opts.sign_only;
+ bool replace_orig = opts.replace_orig;
#ifndef USE_PKCS7
- case 'k': use_keyid = CMS_USE_KEYID; break;
+ const unsigned int use_keyid = opts.use_keyid;
#endif
- case -1: break;
- default: format();
- }
- } while (opt != -1);
- argc -= optind;
- argv += optind;
if (argc < 4 || argc > 5)
format();
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 1/8] sign-file: use getopt_long_only for parsing input args Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-08-07 2:35 ` Masahiro Yamada
2023-06-23 14:53 ` [PATCH v7 3/8] sign-file: move file signing logic to its own function Shreenidhi Shedi
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
- Add some more options like help, x509, hashalgo to command line args
- This makes it easy to handle and use command line args wherever needed
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++++-------------
1 file changed, 45 insertions(+), 18 deletions(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index 94228865b6cc..b0f340ea629b 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -215,6 +215,11 @@ static X509 *read_x509(const char *x509_name)
struct cmd_opts {
char *raw_sig_name;
+ char *hash_algo;
+ char *dest_name;
+ char *private_key_name;
+ char *x509_name;
+ char *module_name;
bool save_sig;
bool replace_orig;
bool raw_sig;
@@ -233,6 +238,12 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
#ifndef USE_PKCS7
{"usekeyid", no_argument, 0, 'k'},
#endif
+ {"help", no_argument, 0, 'h'},
+ {"privkey", required_argument, 0, 'i'},
+ {"hashalgo", required_argument, 0, 'a'},
+ {"x509", required_argument, 0, 'x'},
+ {"dest", required_argument, 0, 'd'},
+ {"replaceorig", required_argument, 0, 'r'},
{0, 0, 0, 0}
};
@@ -241,10 +252,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
do {
#ifndef USE_PKCS7
- opt = getopt_long_only(argc, argv, "pds:",
+ opt = getopt_long_only(argc, argv, "hpds:i:a:x:t:r:",
cmd_options, &opt_index);
#else
- opt = getopt_long_only(argc, argv, "pdks:",
+ opt = getopt_long_only(argc, argv, "hpdks:i:a:x:t:r:",
cmd_options, &opt_index);
#endif
switch (opt) {
@@ -268,6 +279,30 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
break;
#endif
+ case 'h':
+ format();
+ break;
+
+ case 'i':
+ opts->private_key_name = optarg;
+ break;
+
+ case 'a':
+ opts->hash_algo = optarg;
+ break;
+
+ case 'x':
+ opts->x509_name = optarg;
+ break;
+
+ case 't':
+ opts->dest_name = optarg;
+ break;
+
+ case 'r':
+ opts->replace_orig = true;
+ break;
+
case -1:
break;
@@ -281,9 +316,6 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
int main(int argc, char **argv)
{
struct module_signature sig_info = { .id_type = PKEY_ID_PKCS7 };
- char *hash_algo = NULL;
- char *private_key_name = NULL;
- char *x509_name, *module_name, *dest_name;
unsigned char buf[4096];
unsigned long module_size, sig_size;
unsigned int use_signed_attrs;
@@ -315,32 +347,27 @@ int main(int argc, char **argv)
argv += optind;
const char *raw_sig_name = opts.raw_sig_name;
+ const char *hash_algo = opts.hash_algo;
+ const char *private_key_name = opts.private_key_name;
+ const char *x509_name = opts.x509_name;
+ const char *module_name = opts.module_name;
const bool save_sig = opts.save_sig;
const bool raw_sig = opts.raw_sig;
const bool sign_only = opts.sign_only;
bool replace_orig = opts.replace_orig;
+ char *dest_name = opts.dest_name;
#ifndef USE_PKCS7
const unsigned int use_keyid = opts.use_keyid;
#endif
- if (argc < 4 || argc > 5)
+ if (!argv[0] || argc != 1)
format();
- if (raw_sig) {
- raw_sig_name = argv[0];
- hash_algo = argv[1];
- } else {
- hash_algo = argv[0];
- private_key_name = argv[1];
- }
- x509_name = argv[2];
- module_name = argv[3];
- if (argc == 5 && strcmp(argv[3], argv[4]) != 0) {
- dest_name = argv[4];
+ if (dest_name && strcmp(argv[0], dest_name)) {
replace_orig = false;
} else {
ERR(asprintf(&dest_name, "%s.~signed~", module_name) < 0,
- "asprintf");
+ "asprintf");
replace_orig = true;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.
2023-06-23 14:53 ` [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy Shreenidhi Shedi
@ 2023-08-07 2:35 ` Masahiro Yamada
2023-08-07 7:57 ` Shreenidhi Shedi
0 siblings, 1 reply; 19+ messages in thread
From: Masahiro Yamada @ 2023-08-07 2:35 UTC (permalink / raw)
To: Shreenidhi Shedi
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>
> - Add some more options like help, x509, hashalgo to command line args
> - This makes it easy to handle and use command line args wherever needed
>
> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
> ---
> scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/sign-file.c b/scripts/sign-file.c
> index 94228865b6cc..b0f340ea629b 100644
> --- a/scripts/sign-file.c
> +++ b/scripts/sign-file.c
> @@ -215,6 +215,11 @@ static X509 *read_x509(const char *x509_name)
>
> struct cmd_opts {
> char *raw_sig_name;
> + char *hash_algo;
> + char *dest_name;
> + char *private_key_name;
> + char *x509_name;
> + char *module_name;
> bool save_sig;
> bool replace_orig;
> bool raw_sig;
> @@ -233,6 +238,12 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
> #ifndef USE_PKCS7
> {"usekeyid", no_argument, 0, 'k'},
> #endif
> + {"help", no_argument, 0, 'h'},
> + {"privkey", required_argument, 0, 'i'},
> + {"hashalgo", required_argument, 0, 'a'},
> + {"x509", required_argument, 0, 'x'},
> + {"dest", required_argument, 0, 'd'},
> + {"replaceorig", required_argument, 0, 'r'},
> {0, 0, 0, 0}
> };
>
> @@ -241,10 +252,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
>
> do {
> #ifndef USE_PKCS7
> - opt = getopt_long_only(argc, argv, "pds:",
> + opt = getopt_long_only(argc, argv, "hpds:i:a:x:t:r:",
> cmd_options, &opt_index);
> #else
> - opt = getopt_long_only(argc, argv, "pdks:",
> + opt = getopt_long_only(argc, argv, "hpdks:i:a:x:t:r:",
> cmd_options, &opt_index);
> #endif
> switch (opt) {
> @@ -268,6 +279,30 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
> break;
> #endif
>
> + case 'h':
> + format();
> + break;
> +
> + case 'i':
> + opts->private_key_name = optarg;
> + break;
> +
> + case 'a':
> + opts->hash_algo = optarg;
> + break;
> +
> + case 'x':
> + opts->x509_name = optarg;
> + break;
> +
> + case 't':
> + opts->dest_name = optarg;
> + break;
> +
> + case 'r':
> + opts->replace_orig = true;
> + break;
> +
> case -1:
> break;
>
> @@ -281,9 +316,6 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
> int main(int argc, char **argv)
> {
> struct module_signature sig_info = { .id_type = PKEY_ID_PKCS7 };
> - char *hash_algo = NULL;
> - char *private_key_name = NULL;
> - char *x509_name, *module_name, *dest_name;
> unsigned char buf[4096];
> unsigned long module_size, sig_size;
> unsigned int use_signed_attrs;
> @@ -315,32 +347,27 @@ int main(int argc, char **argv)
> argv += optind;
>
> const char *raw_sig_name = opts.raw_sig_name;
> + const char *hash_algo = opts.hash_algo;
> + const char *private_key_name = opts.private_key_name;
> + const char *x509_name = opts.x509_name;
> + const char *module_name = opts.module_name;
> const bool save_sig = opts.save_sig;
> const bool raw_sig = opts.raw_sig;
> const bool sign_only = opts.sign_only;
> bool replace_orig = opts.replace_orig;
> + char *dest_name = opts.dest_name;
> #ifndef USE_PKCS7
> const unsigned int use_keyid = opts.use_keyid;
> #endif
>
> - if (argc < 4 || argc > 5)
> + if (!argv[0] || argc != 1)
> format();
You are breaking the bisect'ability.
You are turning the positional parameters into options
but not adjusting scripts/Makefile.modinst in the same commit.
masahiro@oscar:~/ref/linux((HEAD detached at 41cb7c94595d))$ make
INSTALL_MOD_PATH=/tmp/modules modules_install
INSTALL /tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko
SIGN /tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko
Usage: scripts/sign-file [OPTIONS]... [MODULE]...
Available options:
-h, --help Print this help message and exit
Optional args:
-s, --rawsig <sig> Raw signature
-p, --savesig Save signature
-d, --signonly Sign only
-k, --usekeyid Use key ID
-b, --bulksign Sign modules in bulk
-r, --replaceorig Replace original
-t, --dest <dest> Destination path (Exclusive with bulk option)
Mandatory args:
-i, --privkey <key> Private key
-a, --hashalgo <alg> Hash algorithm
-x, --x509 <x509> X509
Examples:
Regular signing:
scripts/sign-file -a sha512 -i certs/signing_key.pem -x
certs/signing_key.x509 <module>
Signing with destination path:
scripts/sign-file -a sha512 -i certs/signing_key.pem -x
certs/signing_key.x509 <module> -t <path>
Signing modules in bulk:
scripts/sign-file -a sha512 -i certs/signing_key.pem -x
certs/signing_key.x509 -b <module1> <module2> ...
make[2]: *** [scripts/Makefile.modinst:87:
/tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko]
Error 2
make[2]: *** Deleting file
'/tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko'
make[1]: *** [/home/masahiro/ref/linux/Makefile:1964: modules_install] Error 2
make: *** [Makefile:234: __sub-make] Error 2
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.
2023-08-07 2:35 ` Masahiro Yamada
@ 2023-08-07 7:57 ` Shreenidhi Shedi
0 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-08-07 7:57 UTC (permalink / raw)
To: Masahiro Yamada
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On 07/08/23 08:05, Masahiro Yamada wrote:
> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>>
>> - Add some more options like help, x509, hashalgo to command line args
>> - This makes it easy to handle and use command line args wherever needed
>>
>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
>> ---
>> scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 45 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/sign-file.c b/scripts/sign-file.c
>> index 94228865b6cc..b0f340ea629b 100644
>> --- a/scripts/sign-file.c
>> +++ b/scripts/sign-file.c
>> @@ -215,6 +215,11 @@ static X509 *read_x509(const char *x509_name)
>>
>> struct cmd_opts {
>> char *raw_sig_name;
>> + char *hash_algo;
>> + char *dest_name;
>> + char *private_key_name;
>> + char *x509_name;
>> + char *module_name;
>> bool save_sig;
>> bool replace_orig;
>> bool raw_sig;
>> @@ -233,6 +238,12 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
>> #ifndef USE_PKCS7
>> {"usekeyid", no_argument, 0, 'k'},
>> #endif
>> + {"help", no_argument, 0, 'h'},
>> + {"privkey", required_argument, 0, 'i'},
>> + {"hashalgo", required_argument, 0, 'a'},
>> + {"x509", required_argument, 0, 'x'},
>> + {"dest", required_argument, 0, 'd'},
>> + {"replaceorig", required_argument, 0, 'r'},
>> {0, 0, 0, 0}
>> };
>>
>> @@ -241,10 +252,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
>>
>> do {
>> #ifndef USE_PKCS7
>> - opt = getopt_long_only(argc, argv, "pds:",
>> + opt = getopt_long_only(argc, argv, "hpds:i:a:x:t:r:",
>> cmd_options, &opt_index);
>> #else
>> - opt = getopt_long_only(argc, argv, "pdks:",
>> + opt = getopt_long_only(argc, argv, "hpdks:i:a:x:t:r:",
>> cmd_options, &opt_index);
>> #endif
>> switch (opt) {
>> @@ -268,6 +279,30 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
>> break;
>> #endif
>>
>> + case 'h':
>> + format();
>> + break;
>> +
>> + case 'i':
>> + opts->private_key_name = optarg;
>> + break;
>> +
>> + case 'a':
>> + opts->hash_algo = optarg;
>> + break;
>> +
>> + case 'x':
>> + opts->x509_name = optarg;
>> + break;
>> +
>> + case 't':
>> + opts->dest_name = optarg;
>> + break;
>> +
>> + case 'r':
>> + opts->replace_orig = true;
>> + break;
>> +
>> case -1:
>> break;
>>
>> @@ -281,9 +316,6 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
>> int main(int argc, char **argv)
>> {
>> struct module_signature sig_info = { .id_type = PKEY_ID_PKCS7 };
>> - char *hash_algo = NULL;
>> - char *private_key_name = NULL;
>> - char *x509_name, *module_name, *dest_name;
>> unsigned char buf[4096];
>> unsigned long module_size, sig_size;
>> unsigned int use_signed_attrs;
>> @@ -315,32 +347,27 @@ int main(int argc, char **argv)
>> argv += optind;
>>
>> const char *raw_sig_name = opts.raw_sig_name;
>> + const char *hash_algo = opts.hash_algo;
>> + const char *private_key_name = opts.private_key_name;
>> + const char *x509_name = opts.x509_name;
>> + const char *module_name = opts.module_name;
>> const bool save_sig = opts.save_sig;
>> const bool raw_sig = opts.raw_sig;
>> const bool sign_only = opts.sign_only;
>> bool replace_orig = opts.replace_orig;
>> + char *dest_name = opts.dest_name;
>> #ifndef USE_PKCS7
>> const unsigned int use_keyid = opts.use_keyid;
>> #endif
>>
>> - if (argc < 4 || argc > 5)
>> + if (!argv[0] || argc != 1)
>> format();
>
>
>
> You are breaking the bisect'ability.
>
> You are turning the positional parameters into options
> but not adjusting scripts/Makefile.modinst in the same commit.
>
>
>
>
>
> masahiro@oscar:~/ref/linux((HEAD detached at 41cb7c94595d))$ make
> INSTALL_MOD_PATH=/tmp/modules modules_install
> INSTALL /tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko
> SIGN /tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko
> Usage: scripts/sign-file [OPTIONS]... [MODULE]...
> Available options:
> -h, --help Print this help message and exit
>
> Optional args:
> -s, --rawsig <sig> Raw signature
> -p, --savesig Save signature
> -d, --signonly Sign only
> -k, --usekeyid Use key ID
> -b, --bulksign Sign modules in bulk
> -r, --replaceorig Replace original
> -t, --dest <dest> Destination path (Exclusive with bulk option)
>
> Mandatory args:
> -i, --privkey <key> Private key
> -a, --hashalgo <alg> Hash algorithm
> -x, --x509 <x509> X509
>
> Examples:
>
> Regular signing:
> scripts/sign-file -a sha512 -i certs/signing_key.pem -x
> certs/signing_key.x509 <module>
>
> Signing with destination path:
> scripts/sign-file -a sha512 -i certs/signing_key.pem -x
> certs/signing_key.x509 <module> -t <path>
>
> Signing modules in bulk:
> scripts/sign-file -a sha512 -i certs/signing_key.pem -x
> certs/signing_key.x509 -b <module1> <module2> ...
> make[2]: *** [scripts/Makefile.modinst:87:
> /tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko]
> Error 2
> make[2]: *** Deleting file
> '/tmp/modules/lib/modules/6.5.0-rc4+/kernel/arch/x86/events/amd/power.ko'
> make[1]: *** [/home/masahiro/ref/linux/Makefile:1964: modules_install] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
>
>
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
Hi Masahiro Yamada,
Thanks for the review. I will fix this. It's hard to keep the commits
small and not breaking the bisect during code refactoring. In this case
it's not a problem. Thanks for this input.
--
Shedi
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v7 3/8] sign-file: move file signing logic to its own function
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 1/8] sign-file: use getopt_long_only for parsing input args Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 4/8] sign-file: add support to sign modules in bulk Shreenidhi Shedi
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
Keep the main function bare minimal and do less in main function.
This patch is pre-work for bulk module signing support.
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 115 +++++++++++++++++++++-----------------------
1 file changed, 54 insertions(+), 61 deletions(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index b0f340ea629b..64d5e00f08e2 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -313,10 +313,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
} while (opt != -1);
}
-int main(int argc, char **argv)
+static int sign_single_file(struct cmd_opts *opts)
{
struct module_signature sig_info = { .id_type = PKEY_ID_PKCS7 };
- unsigned char buf[4096];
+ unsigned char buf[4096] = {};
unsigned long module_size, sig_size;
unsigned int use_signed_attrs;
const EVP_MD *digest_algo;
@@ -329,11 +329,6 @@ int main(int argc, char **argv)
X509 *x509;
BIO *bd, *bm;
int n;
- struct cmd_opts opts = {};
-
- OpenSSL_add_all_algorithms();
- ERR_load_crypto_strings();
- ERR_clear_error();
key_pass = getenv("KBUILD_SIGN_PIN");
@@ -342,34 +337,6 @@ int main(int argc, char **argv)
#else
use_signed_attrs = PKCS7_NOATTR;
#endif
- parse_args(argc, argv, &opts);
- argc -= optind;
- argv += optind;
-
- const char *raw_sig_name = opts.raw_sig_name;
- const char *hash_algo = opts.hash_algo;
- const char *private_key_name = opts.private_key_name;
- const char *x509_name = opts.x509_name;
- const char *module_name = opts.module_name;
- const bool save_sig = opts.save_sig;
- const bool raw_sig = opts.raw_sig;
- const bool sign_only = opts.sign_only;
- bool replace_orig = opts.replace_orig;
- char *dest_name = opts.dest_name;
-#ifndef USE_PKCS7
- const unsigned int use_keyid = opts.use_keyid;
-#endif
-
- if (!argv[0] || argc != 1)
- format();
-
- if (dest_name && strcmp(argv[0], dest_name)) {
- replace_orig = false;
- } else {
- ERR(asprintf(&dest_name, "%s.~signed~", module_name) < 0,
- "asprintf");
- replace_orig = true;
- }
#ifdef USE_PKCS7
if (strcmp(hash_algo, "sha1") != 0) {
@@ -380,20 +347,20 @@ int main(int argc, char **argv)
#endif
/* Open the module file */
- bm = BIO_new_file(module_name, "rb");
- ERR(!bm, "%s", module_name);
+ bm = BIO_new_file(opts->module_name, "rb");
+ ERR(!bm, "%s", opts->module_name);
- if (!raw_sig) {
+ if (!opts->raw_sig) {
/* Read the private key and the X.509 cert the PKCS#7 message
* will point to.
*/
- private_key = read_private_key(private_key_name);
- x509 = read_x509(x509_name);
+ private_key = read_private_key(opts->private_key_name);
+ x509 = read_x509(opts->x509_name);
/* Digest the module data. */
OpenSSL_add_all_digests();
display_openssl_errors(__LINE__);
- digest_algo = EVP_get_digestbyname(hash_algo);
+ digest_algo = EVP_get_digestbyname(opts->hash_algo);
ERR(!digest_algo, "EVP_get_digestbyname");
#ifndef USE_PKCS7
@@ -405,7 +372,7 @@ int main(int argc, char **argv)
ERR(!CMS_add1_signer(cms, x509, private_key, digest_algo,
CMS_NOCERTS | CMS_BINARY |
- CMS_NOSMIMECAP | use_keyid |
+ CMS_NOSMIMECAP | opts->use_keyid |
use_signed_attrs),
"CMS_add1_signer");
ERR(CMS_final(cms, bm, NULL, CMS_NOCERTS | CMS_BINARY) < 0,
@@ -418,11 +385,11 @@ int main(int argc, char **argv)
ERR(!pkcs7, "PKCS7_sign");
#endif
- if (save_sig) {
+ if (opts->save_sig) {
char *sig_file_name;
BIO *b;
- ERR(asprintf(&sig_file_name, "%s.p7s", module_name) < 0,
+ ERR(asprintf(&sig_file_name, "%s.p7s", opts->module_name) < 0,
"asprintf");
b = BIO_new_file(sig_file_name, "wb");
ERR(!b, "%s", sig_file_name);
@@ -436,7 +403,7 @@ int main(int argc, char **argv)
BIO_free(b);
}
- if (sign_only) {
+ if (opts->sign_only) {
BIO_free(bm);
return 0;
}
@@ -445,24 +412,24 @@ int main(int argc, char **argv)
/* Open the destination file now so that we can shovel the module data
* across as we read it.
*/
- bd = BIO_new_file(dest_name, "wb");
- ERR(!bd, "%s", dest_name);
+ bd = BIO_new_file(opts->dest_name, "wb");
+ ERR(!bd, "%s", opts->dest_name);
/* Append the marker and the PKCS#7 message to the destination file */
- ERR(BIO_reset(bm) < 0, "%s", module_name);
+ ERR(BIO_reset(bm) < 0, "%s", opts->module_name);
while ((n = BIO_read(bm, buf, sizeof(buf))),
n > 0) {
- ERR(BIO_write(bd, buf, n) < 0, "%s", dest_name);
+ ERR(BIO_write(bd, buf, n) < 0, "%s", opts->dest_name);
}
BIO_free(bm);
- ERR(n < 0, "%s", module_name);
+ ERR(n < 0, "%s", opts->module_name);
module_size = BIO_number_written(bd);
- if (!raw_sig) {
+ if (!opts->raw_sig) {
#ifndef USE_PKCS7
- ERR(i2d_CMS_bio_stream(bd, cms, NULL, 0) < 0, "%s", dest_name);
+ ERR(i2d_CMS_bio_stream(bd, cms, NULL, 0) < 0, "%s", opts->dest_name);
#else
- ERR(i2d_PKCS7_bio(bd, pkcs7) < 0, "%s", dest_name);
+ ERR(i2d_PKCS7_bio(bd, pkcs7) < 0, "%s", opts->dest_name);
#endif
} else {
BIO *b;
@@ -470,23 +437,49 @@ int main(int argc, char **argv)
/* Read the raw signature file and write the data to the
* destination file
*/
- b = BIO_new_file(raw_sig_name, "rb");
- ERR(!b, "%s", raw_sig_name);
+ b = BIO_new_file(opts->raw_sig_name, "rb");
+ ERR(!b, "%s", opts->raw_sig_name);
while ((n = BIO_read(b, buf, sizeof(buf))), n > 0)
- ERR(BIO_write(bd, buf, n) < 0, "%s", dest_name);
+ ERR(BIO_write(bd, buf, n) < 0, "%s", opts->dest_name);
BIO_free(b);
}
sig_size = BIO_number_written(bd) - module_size;
sig_info.sig_len = htonl(sig_size);
- ERR(BIO_write(bd, &sig_info, sizeof(sig_info)) < 0, "%s", dest_name);
- ERR(BIO_write(bd, magic_number, sizeof(magic_number) - 1) < 0, "%s", dest_name);
+ ERR(BIO_write(bd, &sig_info, sizeof(sig_info)) < 0, "%s", opts->dest_name);
+ ERR(BIO_write(bd, magic_number, sizeof(magic_number) - 1) < 0, "%s", opts->dest_name);
- ERR(BIO_free(bd) < 0, "%s", dest_name);
+ ERR(BIO_free(bd) < 0, "%s", opts->dest_name);
/* Finally, if we're signing in place, replace the original. */
- if (replace_orig)
- ERR(rename(dest_name, module_name) < 0, "%s", dest_name);
+ if (opts->replace_orig)
+ ERR(rename(opts->dest_name, opts->module_name) < 0, "%s", opts->dest_name);
return 0;
}
+
+int main(int argc, char **argv)
+{
+ struct cmd_opts opts = {};
+
+ parse_args(argc, argv, &opts);
+ argc -= optind;
+ argv += optind;
+
+ if (!argv[0] || argc != 1)
+ format();
+
+ if (opts.dest_name && strcmp(argv[0], opts.dest_name)) {
+ opts.replace_orig = false;
+ } else {
+ ERR(asprintf(&opts.dest_name, "%s.~signed~", opts.module_name) < 0,
+ "asprintf");
+ opts.replace_orig = true;
+ }
+
+ OpenSSL_add_all_algorithms();
+ ERR_load_crypto_strings();
+ ERR_clear_error();
+
+ return sign_single_file(&opts);
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 4/8] sign-file: add support to sign modules in bulk
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
` (2 preceding siblings ...)
2023-06-23 14:53 ` [PATCH v7 3/8] sign-file: move file signing logic to its own function Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 5/8] sign-file: improve help message Shreenidhi Shedi
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
In the existing system, we need to invoke sign-file binary for every
module we want to sign. This patch adds support to give modules list
in bulk and it will sign them all one by one.
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index 64d5e00f08e2..0a275256ca16 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -224,6 +224,7 @@ struct cmd_opts {
bool replace_orig;
bool raw_sig;
bool sign_only;
+ bool bulk_sign;
#ifndef USE_PKCS7
unsigned int use_keyid;
#endif
@@ -252,10 +253,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
do {
#ifndef USE_PKCS7
- opt = getopt_long_only(argc, argv, "hpds:i:a:x:t:r:",
+ opt = getopt_long_only(argc, argv, "hpdbs:i:a:x:t:r:",
cmd_options, &opt_index);
#else
- opt = getopt_long_only(argc, argv, "hpdks:i:a:x:t:r:",
+ opt = getopt_long_only(argc, argv, "hpdkbs:i:a:x:t:r:",
cmd_options, &opt_index);
#endif
switch (opt) {
@@ -303,6 +304,10 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
opts->replace_orig = true;
break;
+ case 'b':
+ opts->bulk_sign = true;
+ break;
+
case -1:
break;
@@ -460,26 +465,34 @@ static int sign_single_file(struct cmd_opts *opts)
int main(int argc, char **argv)
{
+ int i;
struct cmd_opts opts = {};
parse_args(argc, argv, &opts);
argc -= optind;
argv += optind;
- if (!argv[0] || argc != 1)
- format();
-
- if (opts.dest_name && strcmp(argv[0], opts.dest_name)) {
- opts.replace_orig = false;
- } else {
- ERR(asprintf(&opts.dest_name, "%s.~signed~", opts.module_name) < 0,
- "asprintf");
- opts.replace_orig = true;
- }
-
OpenSSL_add_all_algorithms();
ERR_load_crypto_strings();
ERR_clear_error();
- return sign_single_file(&opts);
+ for (i = 0; i < argc; ++i) {
+ opts.module_name = argv[i];
+
+ if (!opts.bulk_sign && opts.dest_name && strcmp(argv[i], opts.dest_name)) {
+ opts.replace_orig = false;
+ } else {
+ ERR(asprintf(&opts.dest_name, "%s.~signed~", opts.module_name) < 0,
+ "asprintf");
+ if (!opts.replace_orig)
+ opts.replace_orig = true;
+ }
+
+ if (sign_single_file(&opts)) {
+ fprintf(stderr, "Failed to sign: %s module\n", opts.module_name);
+ return -1;
+ }
+ }
+
+ return 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 5/8] sign-file: improve help message
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
` (3 preceding siblings ...)
2023-06-23 14:53 ` [PATCH v7 4/8] sign-file: add support to sign modules in bulk Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 6/8] sign-file: use const with a global string constant Shreenidhi Shedi
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
Add a proper help message with examples on how to use this tool.
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 48 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index 0a275256ca16..d3abc5721a7e 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -74,12 +74,43 @@ struct module_signature {
static char magic_number[] = "~Module signature appended~\n";
static __attribute__((noreturn))
-void format(void)
+void print_usage(void)
{
- fprintf(stderr,
- "Usage: scripts/sign-file [-dp] <hash algo> <key> <x509> <module> [<dest>]\n");
- fprintf(stderr,
- " scripts/sign-file -s <raw sig> <hash algo> <x509> <module> [<dest>]\n");
+ fprintf(stderr, "Usage: scripts/sign-file [OPTIONS]... [MODULE]...\n");
+ fprintf(stderr, "Available options:\n");
+ fprintf(stderr, "-h, --help Print this help message and exit\n");
+
+ fprintf(stderr, "\nOptional args:\n");
+ fprintf(stderr, "-s, --rawsig <sig> Raw signature\n");
+ fprintf(stderr, "-p, --savesig Save signature\n");
+ fprintf(stderr, "-d, --signonly Sign only\n");
+#ifndef USE_PKCS7
+ fprintf(stderr, "-k, --usekeyid Use key ID\n");
+#endif
+ fprintf(stderr, "-b, --bulksign Sign modules in bulk\n");
+ fprintf(stderr, "-r, --replaceorig Replace original\n");
+ fprintf(stderr, "-t, --dest <dest> Destination path ");
+ fprintf(stderr, "(Exclusive with bulk option)\n");
+
+ fprintf(stderr, "\nMandatory args:\n");
+ fprintf(stderr, "-i, --privkey <key> Private key\n");
+ fprintf(stderr, "-a, --hashalgo <alg> Hash algorithm\n");
+ fprintf(stderr, "-x, --x509 <x509> X509\n");
+
+ fprintf(stderr, "\nExamples:\n");
+
+ fprintf(stderr, "\n Regular signing:\n");
+ fprintf(stderr, " scripts/sign-file -a sha512 -i certs/signing_key.pem ");
+ fprintf(stderr, "-x certs/signing_key.x509 <module>\n");
+
+ fprintf(stderr, "\n Signing with destination path:\n");
+ fprintf(stderr, " scripts/sign-file -a sha512 -i certs/signing_key.pem ");
+ fprintf(stderr, "-x certs/signing_key.x509 <module> -t <path>\n");
+
+ fprintf(stderr, "\n Signing modules in bulk:\n");
+ fprintf(stderr, " scripts/sign-file -a sha512 -i certs/signing_key.pem ");
+ fprintf(stderr, "-x certs/signing_key.x509 -b <module1> <module2> ...\n");
+
exit(2);
}
@@ -281,7 +312,7 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
#endif
case 'h':
- format();
+ print_usage();
break;
case 'i':
@@ -312,7 +343,7 @@ static void parse_args(int argc, char **argv, struct cmd_opts *opts)
break;
default:
- format();
+ print_usage();
break;
}
} while (opt != -1);
@@ -472,6 +503,9 @@ int main(int argc, char **argv)
argc -= optind;
argv += optind;
+ if ((opts.bulk_sign && opts.dest_name) || (!opts.bulk_sign && argc != 1))
+ print_usage();
+
OpenSSL_add_all_algorithms();
ERR_load_crypto_strings();
ERR_clear_error();
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 6/8] sign-file: use const with a global string constant
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
` (4 preceding siblings ...)
2023-06-23 14:53 ` [PATCH v7 5/8] sign-file: improve help message Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 7/8] sign-file: fix do while styling issue Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 8/8] kbuild: modinst: do modules_install step by step Shreenidhi Shedi
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
Reported by checkpatch.
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index d3abc5721a7e..e8dfbdd3eea3 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -71,7 +71,7 @@ struct module_signature {
#define PKEY_ID_PKCS7 2
-static char magic_number[] = "~Module signature appended~\n";
+static const char magic_number[] = "~Module signature appended~\n";
static __attribute__((noreturn))
void print_usage(void)
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 7/8] sign-file: fix do while styling issue
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
` (5 preceding siblings ...)
2023-06-23 14:53 ` [PATCH v7 6/8] sign-file: use const with a global string constant Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-06-23 14:53 ` [PATCH v7 8/8] kbuild: modinst: do modules_install step by step Shreenidhi Shedi
7 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
Reported by checkpatch.
Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
scripts/sign-file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index e8dfbdd3eea3..0c95275c4564 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -147,7 +147,7 @@ static void drain_openssl_errors(void)
if (__cond) { \
errx(1, fmt, ## __VA_ARGS__); \
} \
- } while(0)
+ } while (0)
static const char *key_pass;
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-06-23 14:53 [PATCH v7 0/8] refactor file signing program Shreenidhi Shedi
` (6 preceding siblings ...)
2023-06-23 14:53 ` [PATCH v7 7/8] sign-file: fix do while styling issue Shreenidhi Shedi
@ 2023-06-23 14:53 ` Shreenidhi Shedi
2023-08-04 14:06 ` Greg KH
2023-08-06 19:32 ` Masahiro Yamada
7 siblings, 2 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 14:53 UTC (permalink / raw)
To: dhowells, dwmw2, gregkh, masahiroy, nathan, ndesaulniers, nicolas
Cc: yesshedi, linux-kernel, sshedi
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@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-06-23 14:53 ` [PATCH v7 8/8] kbuild: modinst: do modules_install step by step Shreenidhi Shedi
@ 2023-08-04 14:06 ` Greg KH
2023-08-05 19:00 ` Shreenidhi Shedi
2023-08-06 19:32 ` Masahiro Yamada
1 sibling, 1 reply; 19+ messages in thread
From: Greg KH @ 2023-08-04 14:06 UTC (permalink / raw)
To: Shreenidhi Shedi
Cc: dhowells, dwmw2, masahiroy, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi 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@gmail.com>
> ---
> 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
As you are touching the build process, you should always cc: the proper
mailing list, and the KBUILD maintainer. Please do so for this series,
as that is the proper tree for this to go through.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-04 14:06 ` Greg KH
@ 2023-08-05 19:00 ` Shreenidhi Shedi
2023-08-06 6:45 ` Greg KH
0 siblings, 1 reply; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-08-05 19:00 UTC (permalink / raw)
To: Greg KH
Cc: dhowells, dwmw2, masahiroy, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi, linux-kbuild
On 04/08/23 19:36, Greg KH wrote:
> On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi 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@gmail.com>
>> ---
>> 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
>
> As you are touching the build process, you should always cc: the proper
> mailing list, and the KBUILD maintainer. Please do so for this series,
> as that is the proper tree for this to go through.
>
> thanks,
>
> greg k-h
Thanks for the inputs Greg.
CC-ing linux-kbuild@vger.kernel.org as suggested.
--
Shedi
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-05 19:00 ` Shreenidhi Shedi
@ 2023-08-06 6:45 ` Greg KH
2023-08-07 11:18 ` Shreenidhi Shedi
0 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2023-08-06 6:45 UTC (permalink / raw)
To: Shreenidhi Shedi
Cc: dhowells, dwmw2, masahiroy, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi, linux-kbuild
On Sun, Aug 06, 2023 at 12:30:22AM +0530, Shreenidhi Shedi wrote:
> On 04/08/23 19:36, Greg KH wrote:
> > On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi 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@gmail.com>
> > > ---
> > > 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
> >
> > As you are touching the build process, you should always cc: the proper
> > mailing list, and the KBUILD maintainer. Please do so for this series,
> > as that is the proper tree for this to go through.
> >
> > thanks,
> >
> > greg k-h
>
> Thanks for the inputs Greg.
>
> CC-ing linux-kbuild@vger.kernel.org as suggested.
This doesn't actually do anything, sorry. Please resend the whole
patchset again and add the proper people and list.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-06 6:45 ` Greg KH
@ 2023-08-07 11:18 ` Shreenidhi Shedi
0 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-08-07 11:18 UTC (permalink / raw)
To: Greg KH
Cc: dhowells, dwmw2, masahiroy, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi, linux-kbuild
On 06/08/23 12:15, Greg KH wrote:
> On Sun, Aug 06, 2023 at 12:30:22AM +0530, Shreenidhi Shedi wrote:
>> On 04/08/23 19:36, Greg KH wrote:
>>> On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi 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@gmail.com>
>>>> ---
>>>> 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
>>>
>>> As you are touching the build process, you should always cc: the proper
>>> mailing list, and the KBUILD maintainer. Please do so for this series,
>>> as that is the proper tree for this to go through.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Thanks for the inputs Greg.
>>
>> CC-ing linux-kbuild@vger.kernel.org as suggested.
>
> This doesn't actually do anything, sorry. Please resend the whole
> patchset again and add the proper people and list.
>
> thanks,
>
> greg k-h
Done. Addressed comments from Masahiro Yamada and sent a new patch
series, hopefully I have added everyone this time :) Thanks.
--
Shedi
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-06-23 14:53 ` [PATCH v7 8/8] kbuild: modinst: do modules_install step by step Shreenidhi Shedi
2023-08-04 14:06 ` Greg KH
@ 2023-08-06 19:32 ` Masahiro Yamada
2023-08-07 8:08 ` Shreenidhi Shedi
1 sibling, 1 reply; 19+ messages in thread
From: Masahiro Yamada @ 2023-08-06 19:32 UTC (permalink / raw)
To: Shreenidhi Shedi
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.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@gmail.com>
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-06 19:32 ` Masahiro Yamada
@ 2023-08-07 8:08 ` Shreenidhi Shedi
2023-08-07 18:44 ` Masahiro Yamada
0 siblings, 1 reply; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-08-07 8:08 UTC (permalink / raw)
To: Masahiro Yamada
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On 07/08/23 01:02, Masahiro Yamada wrote:
> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.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@gmail.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@ph5dev:~/linux-6.3.5 # ./test.sh orig
real 0m14.699s
user 0m55.519s
sys 0m9.036s
root@ph5dev:~/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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-07 8:08 ` Shreenidhi Shedi
@ 2023-08-07 18:44 ` Masahiro Yamada
2023-08-09 17:27 ` Shreenidhi Shedi
0 siblings, 1 reply; 19+ messages in thread
From: Masahiro Yamada @ 2023-08-07 18:44 UTC (permalink / raw)
To: Shreenidhi Shedi
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>
> On 07/08/23 01:02, Masahiro Yamada wrote:
> > On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.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@gmail.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.
The existing code runs in parallel.
1. Copy the module "foo.ko" to the destination
2. Sign the module "bar.ko"
3. Compress the module "baz.ko"
Those three have no dependency among them, so
should be able to run in parallel.
Your code serializes 1 -> 2 -> 3
> And the existing
> method takes more time than the proposed version.
>
> root@ph5dev:~/linux-6.3.5 # ./test.sh orig
>
> real 0m14.699s
> user 0m55.519s
> sys 0m9.036s
>
> root@ph5dev:~/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.
I do not understand why "closer to Unix philosophy"?
You are adding extra/unnecessary complexity.
Currently, the parallel job is managed by Make's job server.
You are introducing another way of parallel execution
in scripts/signfile.sh
(and you completely ignored -j <jobs> option to Make,
and always spawned $(nproc) threads).
Leave the parallel execution GNU Make.
That is how Kbuild works _properly_.
> > 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.
I saw it. I re-confirmed this is not an improvement. Thanks for the data.
As I replied to the other thread, my measurement did not show an
attractive result.
https://lore.kernel.org/lkml/CAK7LNATNRchNoj0Y6sdb+_81xwV3kAX57-w5q2zew-q8RyzJVg@mail.gmail.com/T/#m8234fc76e631363fbe6bdfb6e45ef6727fc48e80
>
> > 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.
I must let you know you are misunderstanding
the meaning of NACK.
NACK means:
"I do not like it. Please do not submit it any more".
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
2023-08-07 18:44 ` Masahiro Yamada
@ 2023-08-09 17:27 ` Shreenidhi Shedi
0 siblings, 0 replies; 19+ messages in thread
From: Shreenidhi Shedi @ 2023-08-09 17:27 UTC (permalink / raw)
To: Masahiro Yamada
Cc: dhowells, dwmw2, gregkh, nathan, ndesaulniers, nicolas,
linux-kernel, sshedi
On 08/08/23 00:14, Masahiro Yamada wrote:
> On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>>
>> On 07/08/23 01:02, Masahiro Yamada wrote:
>>> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.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@gmail.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.
>
> The existing code runs in parallel.
>
> 1. Copy the module "foo.ko" to the destination
> 2. Sign the module "bar.ko"
> 3. Compress the module "baz.ko"
>
> Those three have no dependency among them, so
> should be able to run in parallel.
>
> Your code serializes 1 -> 2 -> 3
>
>
>
>> And the existing
>> method takes more time than the proposed version.
>>
>> root@ph5dev:~/linux-6.3.5 # ./test.sh orig
>>
>> real 0m14.699s
>> user 0m55.519s
>> sys 0m9.036s
>>
>> root@ph5dev:~/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.
>
>
> I do not understand why "closer to Unix philosophy"?
>
> You are adding extra/unnecessary complexity.
>
> Currently, the parallel job is managed by Make's job server.
>
> You are introducing another way of parallel execution
> in scripts/signfile.sh
> (and you completely ignored -j <jobs> option to Make,
> and always spawned $(nproc) threads).
>
>
> Leave the parallel execution GNU Make.
> That is how Kbuild works _properly_.
>
>
>
>
>>> 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.
>
>
> I saw it. I re-confirmed this is not an improvement. Thanks for the data.
>
> As I replied to the other thread, my measurement did not show an
> attractive result.
> https://lore.kernel.org/lkml/CAK7LNATNRchNoj0Y6sdb+_81xwV3kAX57-w5q2zew-q8RyzJVg@mail.gmail.com/T/#m8234fc76e631363fbe6bdfb6e45ef6727fc48e80
>
>
>>
>>> 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.
>
>
> I must let you know you are misunderstanding
> the meaning of NACK.
>
>
> NACK means:
> "I do not like it. Please do not submit it any more".
>
>
>
> --
> Best Regards
> Masahiro Yamada
Thanks for all the advice and your time, I'm getting used to this
process. I dropped Kbuild changes from my patch series v9. PTAL.
Have a nice time ahead.
--
Shedi
^ permalink raw reply [flat|nested] 19+ messages in thread