[PATCH v7 0/8] refactor file signing program

[PATCH v7 0/8] refactor file signing program

[Shreenidhi Shedi]

This patch series refactors the sign-file program.

Brief of changes in this patch series:

- Improve argument parsing logic.
- Add few more easy to remember arguments.
- Add support to sign bunch of modules at once.
- Improve the help message with examples.
- Few trivial checkpatch reported issue fixes.
- Divide the modules_install task into sub tasks

Version 7 changes:
- Change Makefile.modinst and divide the tasks further
- Don't do everything from one place.
- This whole thing is done to facilitate bulk signing of modules
- Greg suggsted this idea here:
https://lore.kernel.org/all/2023060155-mustard-mating-32b7@gregkh/
- Thanks for the inputs Greg
- v7-0008-kbuild-modinst-do-modules_install-step-by-step.patch is fairly
big and I'm sorry about it. I created all patches considering build
stability in mind, so this can't be broken into pieces else in the
intermediate commit build will break.

Version 6 changes:
- Fix commit messages as suggested by Greg and David.

Version 5 changes:
- Addressed review comments from David Howells.
- Fragmented the patches into further small units.
Link:
v4: https://lore.kernel.org/all/20230221170804.3267242-1-yesshedi@gmail.com/

Version 1 - Version 4 changes:
Did some back and forth changes. Getting familiar with patch submission
process, nothing significant happened.

Links:
v1: https://lore.kernel.org/all/dc852d8e-816a-0fb2-f50e-ff6c2aa11dd8@gmail.com/
v2: https://lore.kernel.org/all/20230213185019.56902-1-yesshedi@gmail.com/
v3: https://lore.kernel.org/all/20230213190034.57097-1-yesshedi@gmail.com/

Shreenidhi Shedi (8):
  sign-file: use getopt_long_only for parsing input args
  sign-file: inntroduce few new flags to make argument processing easy.
  sign-file: move file signing logic to its own function
  sign-file: add support to sign modules in bulk
  sign-file: improve help message
  sign-file: use const with a global string constant
  sign-file: fix do while styling issue
  kbuild: modinst: do modules_install step by step

 scripts/Makefile.compress |  53 +++++++
 scripts/Makefile.install  |  66 +++++++++
 scripts/Makefile.modinst  | 111 +--------------
 scripts/Makefile.sign     |  37 +++++
 scripts/sign-file.c       | 292 +++++++++++++++++++++++++++-----------
 scripts/signfile.sh       |  24 ++++
 6 files changed, 395 insertions(+), 188 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

--
2.41.0

[PATCH v7 1/8] sign-file: use getopt_long_only for parsing input args

[Shreenidhi Shedi]

- 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

[PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.

[Shreenidhi Shedi]

- 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

[PATCH v7 3/8] sign-file: move file signing logic to its own function

[Shreenidhi Shedi]

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

[PATCH v7 4/8] sign-file: add support to sign modules in bulk

[Shreenidhi Shedi]

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

[PATCH v7 5/8] sign-file: improve help message

[Shreenidhi Shedi]

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

[PATCH v7 6/8] sign-file: use const with a global string constant

[Shreenidhi Shedi]

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

[PATCH v7 7/8] sign-file: fix do while styling issue

[Shreenidhi Shedi]

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

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

[Shreenidhi Shedi]

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

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

[Greg KH]

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

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

[Shreenidhi Shedi]

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

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

[Greg KH]

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

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

[Masahiro Yamada]

On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@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

Re: [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.

[Masahiro Yamada]

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

Re: [PATCH v7 2/8] sign-file: inntroduce few new flags to make argument processing easy.

[Shreenidhi Shedi]

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

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

[Shreenidhi Shedi]

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

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

[Shreenidhi Shedi]

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

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

[Masahiro Yamada]

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

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

[Shreenidhi Shedi]

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