* [PATCH v6 1/7] sign-file: use getopt_long_only for parsing input args
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 2/7] sign-file: inntroduce few new flags to make argument processing easy Shreenidhi Shedi
` (6 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
- 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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 2/7] sign-file: inntroduce few new flags to make argument processing easy.
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 1/7] sign-file: use getopt_long_only for parsing input args Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 3/7] sign-file: move file signing logic to its own function Shreenidhi Shedi
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
- 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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 3/7] sign-file: move file signing logic to its own function
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 1/7] sign-file: use getopt_long_only for parsing input args Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 2/7] sign-file: inntroduce few new flags to make argument processing easy Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 4/7] sign-file: add support to sign modules in bulk Shreenidhi Shedi
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 4/7] sign-file: add support to sign modules in bulk
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
` (2 preceding siblings ...)
2023-03-21 19:33 ` [PATCH v6 3/7] sign-file: move file signing logic to its own function Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 5/7] sign-file: improve help message Shreenidhi Shedi
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 5/7] sign-file: improve help message
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
` (3 preceding siblings ...)
2023-03-21 19:33 ` [PATCH v6 4/7] sign-file: add support to sign modules in bulk Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 6/7] sign-file: use const with a global string constant Shreenidhi Shedi
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 6/7] sign-file: use const with a global string constant
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
` (4 preceding siblings ...)
2023-03-21 19:33 ` [PATCH v6 5/7] sign-file: improve help message Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-03-21 19:33 ` [PATCH v6 7/7] sign-file: fix do while styling issue Shreenidhi Shedi
2023-04-25 10:44 ` [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 7/7] sign-file: fix do while styling issue
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
` (5 preceding siblings ...)
2023-03-21 19:33 ` [PATCH v6 6/7] sign-file: use const with a global string constant Shreenidhi Shedi
@ 2023-03-21 19:33 ` Shreenidhi Shedi
2023-04-25 10:44 ` [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
7 siblings, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-03-21 19:33 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi, yesshedi
From: Shreenidhi Shedi <yesshedi@gmail.com>
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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/7] refactor file signing program
2023-03-21 19:33 [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
` (6 preceding siblings ...)
2023-03-21 19:33 ` [PATCH v6 7/7] sign-file: fix do while styling issue Shreenidhi Shedi
@ 2023-04-25 10:44 ` Shreenidhi Shedi
2023-05-31 14:38 ` Greg KH
7 siblings, 1 reply; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-04-25 10:44 UTC (permalink / raw)
To: gregkh, dhowells, dwmw2; +Cc: linux-kernel, sshedi
On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> From: Shreenidhi Shedi <yesshedi@gmail.com>
>
> 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.
>
> Version 6 changes:
> - Fixed 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 (7):
> 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
>
> scripts/sign-file.c | 292 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 209 insertions(+), 83 deletions(-)
>
> --
> 2.39.2
>
> From mboxrd@z Thu Jan 1 00:00:00 1970
> Return-Path: <linux-kernel-owner@vger.kernel.org>
> X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
> aws-us-west-2-korg-lkml-1.web.codeaurora.org
> Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
> by smtp.lore.kernel.org (Postfix) with ESMTP id 04233C6FD1D
> for <linux-kernel@archiver.kernel.org>; Tue, 21 Mar 2023 19:34:57 +0000 (UTC)
> Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
> id S230310AbjCUTez (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
> Tue, 21 Mar 2023 15:34:55 -0400
> Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50370 "EHLO
> lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
> with ESMTP id S230287AbjCUTeq (ORCPT
> <rfc822;linux-kernel@vger.kernel.org>);
> Tue, 21 Mar 2023 15:34:46 -0400
> Received: from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com [IPv6:2607:f8b0:4864:20::42d])
> by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 127EC570B5
> for <linux-kernel@vger.kernel.org>; Tue, 21 Mar 2023 12:34:09 -0700 (PDT)
> Received: by mail-pf1-x42d.google.com with SMTP id fd25so9747574pfb.1
> for <linux-kernel@vger.kernel.org>; Tue, 21 Mar 2023 12:34:09 -0700 (PDT)
> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
> d=gmail.com; s=20210112; t=1679427226;
> h=content-transfer-encoding:mime-version:references:in-reply-to
> :message-id:date:subject:cc:to:from:from:to:cc:subject:date
> :message-id:reply-to;
> bh=JfE1Pm3xCC/xMjfmbV6dg9bDdIIYNa99PYfAs69HM0w=;
> b=lg/FcqI+lffJF0M/bbmFlheKKJUVTXCS5F8jAhnrBAvXyA2IqG/9hmNjzvsDp5ngKk
> SDO3W2J+fE6lLOj/TSKcsSfKiFb6PBXyAUEVycnCvhNuN9U4QO10ihmPCnwMX6t+okTd
> V7073khKaNF0l7HH0sODuuxEBuR26SC2Sfr3Ejf/A3DwrerYutz/aKdNC06BGtcx9VTd
> jOqI5hf/s5xGB8YKp8zGdbn0XnRG5QE7Io2dLaEw2EDU6RVp+0sQBepgBPbMNnM1vGVC
> w2gtIizlYIO1WyZAXij+vlqgRARBPm42MVPHtG3mEBeVhkuHvcJl9KuzowBZXUqqcm+P
> ELjQ==
> X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
> d=1e100.net; s=20210112; t=1679427226;
> h=content-transfer-encoding:mime-version:references:in-reply-to
> :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc
> :subject:date:message-id:reply-to;
> bh=JfE1Pm3xCC/xMjfmbV6dg9bDdIIYNa99PYfAs69HM0w=;
> b=EycJZurnMRKaNtbX9dO0lkGbc2874R1xwH37vsERv4GSiRcqjNFXyQNcKfdgoQCLir
> C9Y2TX/5Z1RO8h9Q4jLrVKwd4ET+uxWuartUjIxLWn54dRlyT0iQErQ9D1D9u7WlFcL+
> Rzb54LhQ8OsPRnq5EL6pWlV9kwz1f+vRdhGSLzr9Yh9SgcdmfC795gVip2Q4AqoJtPy5
> qyUK9YLjRALEsrfQ6Dv5qa1YHZgJI0pvT5JGj+mG4ivQA8GohclChNDilLqL4bWjrmMJ
> Tsh3y/gU2tvHVzFFclSnR5aLMeyq/YJ0TeQIY2kfY55La4dcKa/vN4zoInzMJtGSauaD
> 0AyQ==
> X-Gm-Message-State: AO0yUKXAn7Kq+WcFipmZkubkO6+9cgkbmRpOdXeWo0Ec3Ybm6KP4x9H4
> jmstKnTCBbBo/srwNR0LEHc=
> X-Google-Smtp-Source: AK7set+AIpPB2wg+jmk+XWvuY7jaNO6CT8aybg2knfYtPhrLXe9DgrH3ebZsJ6n8B4fdOysRGySkBA==
> X-Received: by 2002:a05:6a00:2e1e:b0:626:2bb0:30d4 with SMTP id fc30-20020a056a002e1e00b006262bb030d4mr1076267pfb.8.1679427226423;
> Tue, 21 Mar 2023 12:33:46 -0700 (PDT)
> Received: from f37.eng.vmware.com ([66.170.99.1])
> by smtp.googlemail.com with ESMTPSA id k23-20020aa790d7000000b006247123adf1sm8843044pfk.143.2023.03.21.12.33.45
> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
> Tue, 21 Mar 2023 12:33:46 -0700 (PDT)
> From: Shreenidhi Shedi <yesshedi@gmail.com>
> X-Google-Original-From: Shreenidhi Shedi <sshedi@vmware.com>
> To: gregkh@linuxfoundation.org, dhowells@redhat.com,
> dwmw2@infradead.org
> Cc: linux-kernel@vger.kernel.org, sshedi@vmware.com, yesshedi@gmail.com
> Subject: [PATCH v6 1/7] sign-file: use getopt_long_only for parsing input args
> Date: Wed, 22 Mar 2023 01:03:35 +0530
> Message-Id: <20230321193341.87997-2-sshedi@vmware.com>
> X-Mailer: git-send-email 2.39.2
> In-Reply-To: <20230321193341.87997-1-sshedi@vmware.com>
> References: <20230321193341.87997-1-sshedi@vmware.com>
> MIME-Version: 1.0
> Content-Transfer-Encoding: 8bit
> Precedence: bulk
> List-ID: <linux-kernel.vger.kernel.org>
> X-Mailing-List: linux-kernel@vger.kernel.org
>
> From: Shreenidhi Shedi <yesshedi@gmail.com>
>
> - 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();
>
Hi Greg and David,
Can you please review the latest patch series? I think I have addressed
your concerns. Thanks.
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/7] refactor file signing program
2023-04-25 10:44 ` [PATCH v6 0/7] refactor file signing program Shreenidhi Shedi
@ 2023-05-31 14:38 ` Greg KH
2023-05-31 15:31 ` Shreenidhi Shedi
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2023-05-31 14:38 UTC (permalink / raw)
To: Shreenidhi Shedi; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> Can you please review the latest patch series? I think I have addressed your
> concerns. Thanks.
The big question is, "who is going to use these new features"? This
tool is only used by the in-kernel build scripts, and if they do not
take advantage of these new options you have added, why are they needed?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-05-31 14:38 ` Greg KH
@ 2023-05-31 15:31 ` Shreenidhi Shedi
2023-05-31 16:50 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-05-31 15:31 UTC (permalink / raw)
To: Greg KH; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Wed, 31-May-2023 20:08, Greg KH wrote:
> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
>> Can you please review the latest patch series? I think I have addressed your
>> concerns. Thanks.
>
> The big question is, "who is going to use these new features"? This
> tool is only used by the in-kernel build scripts, and if they do not
> take advantage of these new options you have added, why are they needed?
>
> thanks,
>
> greg k-h
Hi Greg,
Thanks for the response.
We use it in VMware Photon OS. Following is the link for the same.
https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
If this change goes in, it will give a slight push to our build
performance. Can you please take these changes?
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-05-31 15:31 ` Shreenidhi Shedi
@ 2023-05-31 16:50 ` Greg KH
2023-06-01 9:03 ` Shreenidhi Shedi
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2023-05-31 16:50 UTC (permalink / raw)
To: Shreenidhi Shedi; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
> On Wed, 31-May-2023 20:08, Greg KH wrote:
> > On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
> > > On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> > > Can you please review the latest patch series? I think I have addressed your
> > > concerns. Thanks.
> >
> > The big question is, "who is going to use these new features"? This
> > tool is only used by the in-kernel build scripts, and if they do not
> > take advantage of these new options you have added, why are they needed?
> >
> > thanks,
> >
> > greg k-h
>
> Hi Greg,
>
> Thanks for the response.
>
> We use it in VMware Photon OS. Following is the link for the same.
> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
>
> If this change goes in, it will give a slight push to our build performance.
What exactly do you mean by "slight push"?
> Can you please take these changes?
Why would we take changes for something that will not benifit us at all?
You are asking us to maintain code that only benifits your out-of-tree
usecase, which is not how kernel development works (and you don't want
it to work that way...)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-05-31 16:50 ` Greg KH
@ 2023-06-01 9:03 ` Shreenidhi Shedi
2023-06-01 9:08 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-06-01 9:03 UTC (permalink / raw)
To: Greg KH; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Wed, 31-May-2023 22:20, Greg KH wrote:
> On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
>> On Wed, 31-May-2023 20:08, Greg KH wrote:
>>> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
>>>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
>>>> Can you please review the latest patch series? I think I have addressed your
>>>> concerns. Thanks.
>>>
>>> The big question is, "who is going to use these new features"? This
>>> tool is only used by the in-kernel build scripts, and if they do not
>>> take advantage of these new options you have added, why are they needed?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Hi Greg,
>>
>> Thanks for the response.
>>
>> We use it in VMware Photon OS. Following is the link for the same.
>> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
>>
>> If this change goes in, it will give a slight push to our build performance.
>
> What exactly do you mean by "slight push"?
Instead of invoking the signing tool binary for each module, we can pass
modules in bulk and it will reduce the build time by couple of seconds.
>
>> Can you please take these changes?
>
> Why would we take changes for something that will not benifit us at all?
There were no remarks regarding this change being useful to all in the
earlier review comments so I thought this will make things better.
And it is humanly impossible to create something which will benefit
everyone. And I think this applies for lot of things that are already
present in kernel and being maintained.
> You are asking us to maintain code that only benifits your out-of-tree
> usecase, which is not how kernel development works (and you don't want
> it to work that way...)
No problem, feel free to discard this PR.
Thanks for your time and inputs. Have a nice time ahead.
> thanks,
>
> greg k-h
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-06-01 9:03 ` Shreenidhi Shedi
@ 2023-06-01 9:08 ` Greg KH
2023-06-23 15:00 ` Shreenidhi Shedi
2023-08-07 2:23 ` Masahiro Yamada
0 siblings, 2 replies; 20+ messages in thread
From: Greg KH @ 2023-06-01 9:08 UTC (permalink / raw)
To: Shreenidhi Shedi; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
> On Wed, 31-May-2023 22:20, Greg KH wrote:
> > On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
> > > On Wed, 31-May-2023 20:08, Greg KH wrote:
> > > > On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
> > > > > On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> > > > > Can you please review the latest patch series? I think I have addressed your
> > > > > concerns. Thanks.
> > > >
> > > > The big question is, "who is going to use these new features"? This
> > > > tool is only used by the in-kernel build scripts, and if they do not
> > > > take advantage of these new options you have added, why are they needed?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Hi Greg,
> > >
> > > Thanks for the response.
> > >
> > > We use it in VMware Photon OS. Following is the link for the same.
> > > https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
> > >
> > > If this change goes in, it will give a slight push to our build performance.
> >
> > What exactly do you mean by "slight push"?
>
> Instead of invoking the signing tool binary for each module, we can pass
> modules in bulk and it will reduce the build time by couple of seconds.
Then why not modify the in-kernel build system to also do this, allowing
everyone to save time and money (i.e. energy)?
Why keep the build savings to yourself?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-06-01 9:08 ` Greg KH
@ 2023-06-23 15:00 ` Shreenidhi Shedi
2023-08-07 2:23 ` Masahiro Yamada
1 sibling, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-06-23 15:00 UTC (permalink / raw)
To: Greg KH; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On Thu, 1-Jun-2023 14:38, Greg KH wrote:
> On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
>> On Wed, 31-May-2023 22:20, Greg KH wrote:
>>> On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
>>>> On Wed, 31-May-2023 20:08, Greg KH wrote:
>>>>> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
>>>>>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
>>>>>> Can you please review the latest patch series? I think I have addressed your
>>>>>> concerns. Thanks.
>>>>>
>>>>> The big question is, "who is going to use these new features"? This
>>>>> tool is only used by the in-kernel build scripts, and if they do not
>>>>> take advantage of these new options you have added, why are they needed?
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>
>>>> Hi Greg,
>>>>
>>>> Thanks for the response.
>>>>
>>>> We use it in VMware Photon OS. Following is the link for the same.
>>>> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
>>>>
>>>> If this change goes in, it will give a slight push to our build performance.
>>>
>>> What exactly do you mean by "slight push"?
>>
>> Instead of invoking the signing tool binary for each module, we can pass
>> modules in bulk and it will reduce the build time by couple of seconds.
>
> Then why not modify the in-kernel build system to also do this, allowing
> everyone to save time and money (i.e. energy)?
>
> Why keep the build savings to yourself?
>
> thanks,
>
> greg k-h
You are correct. Sorry, I got busy with some other work.
Thanks for the inputs Greg. I sent a patch for the same, please take a look.
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 0/7] refactor file signing program
2023-06-01 9:08 ` Greg KH
2023-06-23 15:00 ` Shreenidhi Shedi
@ 2023-08-07 2:23 ` Masahiro Yamada
2023-08-07 8:17 ` Shreenidhi Shedi
1 sibling, 1 reply; 20+ messages in thread
From: Masahiro Yamada @ 2023-08-07 2:23 UTC (permalink / raw)
To: Greg KH; +Cc: Shreenidhi Shedi, dhowells, dwmw2, linux-kernel, sshedi
On Thu, Jun 1, 2023 at 6:08 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
> > On Wed, 31-May-2023 22:20, Greg KH wrote:
> > > On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
> > > > On Wed, 31-May-2023 20:08, Greg KH wrote:
> > > > > On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
> > > > > > On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> > > > > > Can you please review the latest patch series? I think I have addressed your
> > > > > > concerns. Thanks.
> > > > >
> > > > > The big question is, "who is going to use these new features"? This
> > > > > tool is only used by the in-kernel build scripts, and if they do not
> > > > > take advantage of these new options you have added, why are they needed?
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > Hi Greg,
> > > >
> > > > Thanks for the response.
> > > >
> > > > We use it in VMware Photon OS. Following is the link for the same.
> > > > https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
> > > >
> > > > If this change goes in, it will give a slight push to our build performance.
> > >
> > > What exactly do you mean by "slight push"?
> >
> > Instead of invoking the signing tool binary for each module, we can pass
> > modules in bulk and it will reduce the build time by couple of seconds.
>
> Then why not modify the in-kernel build system to also do this, allowing
> everyone to save time and money (i.e. energy)?
>
> Why keep the build savings to yourself?
>
> thanks,
>
> greg k-h
If I understand correctly,
"sign-file: add support to sign modules in bulk"
is the only benefit in the patchset.
I tested the bulk option, but I did not see build savings.
My evaluation:
1. 'make allmodconfig all', then 'make modules_install'.
(9476 modules installed)
2. I ran 'perf stat' for single signing vs bulk signing
(5 runs for each).
I changed the -n option in scripts/signfile.sh
A. single sign
Sign one module per scripts/sign-file invocation.
find "${MODULES_PATH}" -name *.ko -type f -print0 | \
xargs -r -0 -P$(nproc) -x -n1 sh -c "..."
Performance counter stats for './signfile-single.sh' (5 runs):
22.33 +- 2.26 seconds time elapsed ( +- 10.12% )
B. bulk sign
Sign 32 modules per scripts/sign-file invocation
find "${MODULES_PATH}" -name *.ko -type f -print0 | \
xargs -r -0 -P$(nproc) -x -n32 sh -c "..."
Performance counter stats for './signfile-bulk.sh' (5 runs):
24.78 +- 3.01 seconds time elapsed ( +- 12.14% )
The bulk option decreases the process forks of scripts/sign-file
but I did not get even "slight push".
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/7] refactor file signing program
2023-08-07 2:23 ` Masahiro Yamada
@ 2023-08-07 8:17 ` Shreenidhi Shedi
2023-08-07 10:21 ` Shreenidhi Shedi
2023-08-07 18:35 ` Masahiro Yamada
0 siblings, 2 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-08-07 8:17 UTC (permalink / raw)
To: Masahiro Yamada, Greg KH; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On 07/08/23 07:53, Masahiro Yamada wrote:
> On Thu, Jun 1, 2023 at 6:08 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
>>> On Wed, 31-May-2023 22:20, Greg KH wrote:
>>>> On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
>>>>> On Wed, 31-May-2023 20:08, Greg KH wrote:
>>>>>> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
>>>>>>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
>>>>>>> Can you please review the latest patch series? I think I have addressed your
>>>>>>> concerns. Thanks.
>>>>>>
>>>>>> The big question is, "who is going to use these new features"? This
>>>>>> tool is only used by the in-kernel build scripts, and if they do not
>>>>>> take advantage of these new options you have added, why are they needed?
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> greg k-h
>>>>>
>>>>> Hi Greg,
>>>>>
>>>>> Thanks for the response.
>>>>>
>>>>> We use it in VMware Photon OS. Following is the link for the same.
>>>>> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
>>>>>
>>>>> If this change goes in, it will give a slight push to our build performance.
>>>>
>>>> What exactly do you mean by "slight push"?
>>>
>>> Instead of invoking the signing tool binary for each module, we can pass
>>> modules in bulk and it will reduce the build time by couple of seconds.
>>
>> Then why not modify the in-kernel build system to also do this, allowing
>> everyone to save time and money (i.e. energy)?
>>
>> Why keep the build savings to yourself?
>>
>> thanks,
>>
>> greg k-h
>
>
> If I understand correctly,
> "sign-file: add support to sign modules in bulk"
> is the only benefit in the patchset.
>
> I tested the bulk option, but I did not see build savings.
>
>
>
> My evaluation:
> 1. 'make allmodconfig all', then 'make modules_install'.
> (9476 modules installed)
>
> 2. I ran 'perf stat' for single signing vs bulk signing
> (5 runs for each).
> I changed the -n option in scripts/signfile.sh
>
>
>
>
> A. single sign
>
> Sign one module per scripts/sign-file invocation.
>
> find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> xargs -r -0 -P$(nproc) -x -n1 sh -c "..."
>
>
>
> Performance counter stats for './signfile-single.sh' (5 runs):
>
> 22.33 +- 2.26 seconds time elapsed ( +- 10.12% )
>
>
>
>
> B. bulk sign
>
> Sign 32 modules per scripts/sign-file invocation
>
> find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> xargs -r -0 -P$(nproc) -x -n32 sh -c "..."
>
>
> Performance counter stats for './signfile-bulk.sh' (5 runs):
>
> 24.78 +- 3.01 seconds time elapsed ( +- 12.14% )
>
>
>
>
> The bulk option decreases the process forks of scripts/sign-file
> but I did not get even "slight push".
>
>
>
That's some really interesting data. I'm surprised that with stand alone
run bulk signing is not performing as expected. Can you give the full
command you used for bulk sign? Reduced number of forks should
eventually lead to getting more done in less time.
But I got ~1.4 seconds boost when I did "make module_install".
I gave the data in my other response as well. Copying the same here
because this has in better context.
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)
```
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/7] refactor file signing program
2023-08-07 8:17 ` Shreenidhi Shedi
@ 2023-08-07 10:21 ` Shreenidhi Shedi
2023-08-07 18:35 ` Masahiro Yamada
1 sibling, 0 replies; 20+ messages in thread
From: Shreenidhi Shedi @ 2023-08-07 10:21 UTC (permalink / raw)
To: Masahiro Yamada, Greg KH; +Cc: dhowells, dwmw2, linux-kernel, sshedi
On 07/08/23 13:47, Shreenidhi Shedi wrote:
> On 07/08/23 07:53, Masahiro Yamada wrote:
>> On Thu, Jun 1, 2023 at 6:08 PM Greg KH <gregkh@linuxfoundation.org>
>> wrote:
>>>
>>> On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
>>>> On Wed, 31-May-2023 22:20, Greg KH wrote:
>>>>> On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
>>>>>> On Wed, 31-May-2023 20:08, Greg KH wrote:
>>>>>>> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
>>>>>>>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
>>>>>>>> Can you please review the latest patch series? I think I have
>>>>>>>> addressed your
>>>>>>>> concerns. Thanks.
>>>>>>>
>>>>>>> The big question is, "who is going to use these new features"? This
>>>>>>> tool is only used by the in-kernel build scripts, and if they do not
>>>>>>> take advantage of these new options you have added, why are they
>>>>>>> needed?
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> greg k-h
>>>>>>
>>>>>> Hi Greg,
>>>>>>
>>>>>> Thanks for the response.
>>>>>>
>>>>>> We use it in VMware Photon OS. Following is the link for the same.
>>>>>> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
>>>>>>
>>>>>> If this change goes in, it will give a slight push to our build
>>>>>> performance.
>>>>>
>>>>> What exactly do you mean by "slight push"?
>>>>
>>>> Instead of invoking the signing tool binary for each module, we can
>>>> pass
>>>> modules in bulk and it will reduce the build time by couple of seconds.
>>>
>>> Then why not modify the in-kernel build system to also do this, allowing
>>> everyone to save time and money (i.e. energy)?
>>>
>>> Why keep the build savings to yourself?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>>
>> If I understand correctly,
>> "sign-file: add support to sign modules in bulk"
>> is the only benefit in the patchset.
>>
>> I tested the bulk option, but I did not see build savings.
>>
>>
>>
>> My evaluation:
>> 1. 'make allmodconfig all', then 'make modules_install'.
>> (9476 modules installed)
>>
>> 2. I ran 'perf stat' for single signing vs bulk signing
>> (5 runs for each).
>> I changed the -n option in scripts/signfile.sh
>>
>>
>>
>>
>> A. single sign
>>
>> Sign one module per scripts/sign-file invocation.
>>
>> find "${MODULES_PATH}" -name *.ko -type f -print0 | \
>> xargs -r -0 -P$(nproc) -x -n1 sh -c "..."
>>
>>
>>
>> Performance counter stats for './signfile-single.sh' (5 runs):
>>
>> 22.33 +- 2.26 seconds time elapsed ( +- 10.12% )
>>
>>
>>
>>
>> B. bulk sign
>>
>> Sign 32 modules per scripts/sign-file invocation
>>
>> find "${MODULES_PATH}" -name *.ko -type f -print0 | \
>> xargs -r -0 -P$(nproc) -x -n32 sh -c "..."
>>
>>
>> Performance counter stats for './signfile-bulk.sh' (5 runs):
>>
>> 24.78 +- 3.01 seconds time elapsed ( +- 12.14% )
>>
>>
>>
>>
>> The bulk option decreases the process forks of scripts/sign-file
>> but I did not get even "slight push".
>>
>>
>>
>
> That's some really interesting data. I'm surprised that with stand alone
> run bulk signing is not performing as expected. Can you give the full
> command you used for bulk sign? Reduced number of forks should
> eventually lead to getting more done in less time.
>
> But I got ~1.4 seconds boost when I did "make module_install".
>
> I gave the data in my other response as well. Copying the same here
> because this has in better context.
>
> 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)
> ```
>
I ran the signfile script again using perf. Almost same as the method
you followed.
I have 991 modules in the target modules directory. Following is the report:
```
root@ph5dev:~/linux-6.3.5 #
perf stat ./signfile.sh sha384 certs/signing_key.pem 1
Performance counter stats for './signfile.sh sha384
certs/signing_key.pem 1':
18,498.62 msec task-clock # 7.901
CPUs utilized
6,211 context-switches # 335.755 /sec
52 cpu-migrations # 2.811 /sec
554,414 page-faults # 29.971 K/sec
2.341202651 seconds time elapsed
14.891415000 seconds user
3.018111000 seconds sys
root@ph5dev:~/linux-6.3.5 #
perf stat ./signfile.sh sha384 certs/signing_key.pem 32
Performance counter stats for './signfile.sh sha384
certs/signing_key.pem 32':
8,397.24 msec task-clock # 7.548
CPUs utilized
1,237 context-switches # 147.310 /sec
0 cpu-migrations # 0.000 /sec
22,529 page-faults # 2.683 K/sec
1.112510013 seconds time elapsed
8.057543000 seconds user
0.323572000 seconds sys
```
And now the interesting part. I tested the time saved with only
modules_sign.
root@ph5dev:~/linux-6.3.5 # ./b.sh new
real 0m1.756s
user 0m8.481s
sys 0m0.553s
root@ph5dev:~/linux-6.3.5 # ./b.sh orig
real 0m3.078s
user 0m16.801s
sys 0m3.096s
root@ph5dev:~/linux-6.3.5 # ./b.sh new
real 0m1.757s
user 0m8.554s
sys 0m0.504s
root@ph5dev:~/linux-6.3.5 # ./b.sh orig
real 0m3.098s
user 0m16.855s
sys 0m3.073s
And signfile.sh script also shows the same. I tweaked it a bit to accept
number of process as another arg.
root@ph5dev:~/linux-6.3.5 #
time ./signfile.sh sha384 certs/signing_key.pem 1
real 0m2.343s
user 0m14.916s
sys 0m2.890s
root@ph5dev:~/linux-6.3.5 #
time ./signfile.sh sha384 certs/signing_key.pem 32
real 0m1.120s
user 0m8.120s
sys 0m0.276s
So, every run is saving ~2 seconds of time. I think something is wrong
in the way you tested. Please check once at your end.
--
Shedi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/7] refactor file signing program
2023-08-07 8:17 ` Shreenidhi Shedi
2023-08-07 10:21 ` Shreenidhi Shedi
@ 2023-08-07 18:35 ` Masahiro Yamada
1 sibling, 0 replies; 20+ messages in thread
From: Masahiro Yamada @ 2023-08-07 18:35 UTC (permalink / raw)
To: Shreenidhi Shedi; +Cc: Greg KH, dhowells, dwmw2, linux-kernel, sshedi
[-- Attachment #1: Type: text/plain, Size: 8416 bytes --]
On Mon, Aug 7, 2023 at 5:17 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>
> On 07/08/23 07:53, Masahiro Yamada wrote:
> > On Thu, Jun 1, 2023 at 6:08 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> On Thu, Jun 01, 2023 at 02:33:23PM +0530, Shreenidhi Shedi wrote:
> >>> On Wed, 31-May-2023 22:20, Greg KH wrote:
> >>>> On Wed, May 31, 2023 at 09:01:24PM +0530, Shreenidhi Shedi wrote:
> >>>>> On Wed, 31-May-2023 20:08, Greg KH wrote:
> >>>>>> On Tue, Apr 25, 2023 at 04:14:49PM +0530, Shreenidhi Shedi wrote:
> >>>>>>> On Wed, 22-Mar-2023 01:03, Shreenidhi Shedi wrote:
> >>>>>>> Can you please review the latest patch series? I think I have addressed your
> >>>>>>> concerns. Thanks.
> >>>>>>
> >>>>>> The big question is, "who is going to use these new features"? This
> >>>>>> tool is only used by the in-kernel build scripts, and if they do not
> >>>>>> take advantage of these new options you have added, why are they needed?
> >>>>>>
> >>>>>> thanks,
> >>>>>>
> >>>>>> greg k-h
> >>>>>
> >>>>> Hi Greg,
> >>>>>
> >>>>> Thanks for the response.
> >>>>>
> >>>>> We use it in VMware Photon OS. Following is the link for the same.
> >>>>> https://github.com/vmware/photon/blob/master/SPECS/linux/spec_install_post.inc#L4
> >>>>>
> >>>>> If this change goes in, it will give a slight push to our build performance.
> >>>>
> >>>> What exactly do you mean by "slight push"?
> >>>
> >>> Instead of invoking the signing tool binary for each module, we can pass
> >>> modules in bulk and it will reduce the build time by couple of seconds.
> >>
> >> Then why not modify the in-kernel build system to also do this, allowing
> >> everyone to save time and money (i.e. energy)?
> >>
> >> Why keep the build savings to yourself?
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> >
> > If I understand correctly,
> > "sign-file: add support to sign modules in bulk"
> > is the only benefit in the patchset.
> >
> > I tested the bulk option, but I did not see build savings.
> >
> >
> >
> > My evaluation:
> > 1. 'make allmodconfig all', then 'make modules_install'.
> > (9476 modules installed)
> >
> > 2. I ran 'perf stat' for single signing vs bulk signing
> > (5 runs for each).
> > I changed the -n option in scripts/signfile.sh
> >
> >
> >
> >
> > A. single sign
> >
> > Sign one module per scripts/sign-file invocation.
> >
> > find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> > xargs -r -0 -P$(nproc) -x -n1 sh -c "..."
> >
> >
> >
> > Performance counter stats for './signfile-single.sh' (5 runs):
> >
> > 22.33 +- 2.26 seconds time elapsed ( +- 10.12% )
> >
> >
> >
> >
> > B. bulk sign
> >
> > Sign 32 modules per scripts/sign-file invocation
> >
> > find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> > xargs -r -0 -P$(nproc) -x -n32 sh -c "..."
> >
> >
> > Performance counter stats for './signfile-bulk.sh' (5 runs):
> >
> > 24.78 +- 3.01 seconds time elapsed ( +- 12.14% )
> >
> >
> >
> >
> > The bulk option decreases the process forks of scripts/sign-file
> > but I did not get even "slight push".
> >
> >
> >
>
> That's some really interesting data. I'm surprised that with stand alone
> run bulk signing is not performing as expected. Can you give the full
> command you used for bulk sign?
Attached.
> Reduced number of forks should
> eventually lead to getting more done in less time.
Not necessarily.
You sign 32 modules per thread.
Assume you have 4 CPUs and 160 modules to sign.
For simplicity, assume every module takes the same time to sign.
[Sign 32 modules per 1 process]
CPU0: <=Sign 32 mods=><=Sign 32 mods=> Finish
CPU1: <=Sign 32 mods=><==== Idle ====>
CPU2: <=Sign 32 mods=><==== Idle ====>
CPU3: <=Sign 32 mods=><==== Idle ====>
[Sign 1 modules per 1 process]
CPU0: <===Sign 40 mods===> Finish
CPU1: <===Sign 40 mods===>
CPU2: <===Sign 40 mods===>
CPU3: <===Sign 40 mods===>
In your approach, if CPU0 eats the last 32 modules
from the command line, the other CPUs end up
just waiting for CPU0 to complete the task.
The more CPUs you have, the more idle CPUs
at the last portion of the signing stage.
Do not try to save such a small cost of process forking.
Leave the task scheduling to GNU Make.
>
> But I got ~1.4 seconds boost when I did "make module_install".
>
> I gave the data in my other response as well. Copying the same here
> because this has in better context.
>
> 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)
> ```
Just in case, I followed your
'time make modules_install' measurement.
allmodconfig based on the mainline.
[Setup]
masahiro@oscar:~$ nproc
24
masahiro@oscar:~$ for ((cpu=0; cpu<$(nproc); cpu++)); do sudo sh -c
"echo performance >
/sys/devices/system/cpu/cpu${cpu}/cpufreq/scaling_governor"; done
[Mainline (3 runs)]
masahiro@oscar:~/ref/linux(master)$ git log -1 --oneline
0108963f14e9 (HEAD -> master, origin/master, origin/HEAD) Merge tag
'v6.5-rc5.vfs.fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs
masahiro@oscar:~/ref/linux(master)$ make -s -j24
masahiro@oscar:~/ref/linux(master)$ wc modules.order
9476 9476 314661 modules.order
masahiro@oscar:~/ref/linux(master)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m21.743s
user 1m49.849s
sys 0m18.345s
masahiro@oscar:~/ref/linux(master)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m22.246s
user 1m49.303s
sys 0m18.390s
masahiro@oscar:~/ref/linux(master)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m28.210s
user 1m46.584s
sys 0m17.678s
[Mainline + your patch set (3 runs)]
masahiro@oscar:~/ref/linux(sig-file)$ git log -9 --oneline
7c95522599c5 (HEAD -> sig-file) kbuild: modinst: do modules_install step by step
1cc212890d9a sign-file: fix do while styling issue
0f9c5c01d378 sign-file: use const with a global string constant
6ddc845c5976 sign-file: improve help message
4573855b7e90 sign-file: add support to sign modules in bulk
f9f0c2c4200c sign-file: move file signing logic to its own function
41cb7c94595d sign-file: inntroduce few new flags to make argument
processing easy.
af85d6eaa481 sign-file: use getopt_long_only for parsing input args
0108963f14e9 (origin/master, origin/HEAD, master) Merge tag
'v6.5-rc5.vfs.fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs
masahiro@oscar:~/ref/linux(sig-file)$ make -s -j24
masahiro@oscar:~/ref/linux(sig-file)$ wc modules.order
9476 9476 314661 modules.order
masahiro@oscar:~/ref/linux(sig-file)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m25.931s
user 1m40.787s
sys 0m8.390s
masahiro@oscar:~/ref/linux(sig-file)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m33.393s
user 1m41.782s
sys 0m8.390s
masahiro@oscar:~/ref/linux(sig-file)$ rm -rf /tmp/modules; sudo sh -c
"echo 1 > /proc/sys/vm/drop_caches"; time make -s -j24
INSTALL_MOD_PATH=/tmp/modules modules_install
real 0m26.311s
user 1m39.205s
sys 0m8.196s
With your apprach, 'sys' is much shorter,
presumably due to less number of process forks.
But, 'real' is not attractive.
--
Best Regards
Masahiro Yamada
[-- Attachment #2: signfile-single.sh --]
[-- Type: application/x-shellscript, Size: 380 bytes --]
[-- Attachment #3: signfile-bulk.sh --]
[-- Type: application/x-shellscript, Size: 381 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread