[PATCH v2 1/4] selftests: kmod: Use variable NAME in kmod_test_0001()

[PATCH v2 1/4] selftests: kmod: Use variable NAME in kmod_test_0001()

[Tiezhu Yang]

Use the variable NAME instead of "\000" directly in kmod_test_0001().

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - just add Acked-by tag

 tools/testing/selftests/kmod/kmod.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 3702dbc..da60c3b 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -341,7 +341,7 @@ kmod_test_0001_driver()
 
 	kmod_defaults_driver
 	config_num_threads 1
-	printf '\000' >"$DIR"/config_test_driver
+	printf $NAME >"$DIR"/config_test_driver
 	config_trigger ${FUNCNAME[0]}
 	config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
 }
@@ -352,7 +352,7 @@ kmod_test_0001_fs()
 
 	kmod_defaults_fs
 	config_num_threads 1
-	printf '\000' >"$DIR"/config_test_fs
+	printf $NAME >"$DIR"/config_test_fs
 	config_trigger ${FUNCNAME[0]}
 	config_expect_result ${FUNCNAME[0]} -EINVAL
 }
-- 
2.1.0

[PATCH v2 2/4] kmod: Remove redundant "be an" in the comment

[Tiezhu Yang]

There exists redundant "be an" in the comment, remove it.

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - just add Acked-by tag

 kernel/kmod.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 37c3c4b..3cd075c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -36,9 +36,8 @@
  *
  * If you need less than 50 threads would mean we're dealing with systems
  * smaller than 3200 pages. This assumes you are capable of having ~13M memory,
- * and this would only be an be an upper limit, after which the OOM killer
- * would take effect. Systems like these are very unlikely if modules are
- * enabled.
+ * and this would only be an upper limit, after which the OOM killer would take
+ * effect. Systems like these are very unlikely if modules are enabled.
  */
 #define MAX_KMOD_CONCURRENT 50
 static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT);
-- 
2.1.0

[PATCH v2 3/4] kmod: Return directly if module name is empty in request_module()

[Tiezhu Yang]

If module name is empty, it is better to return directly at the beginning
of request_module() without doing the needless call_modprobe() operation.

Call trace:

request_module()
      |
      |
__request_module()
      |
      |
call_modprobe()
      |
      |
call_usermodehelper_exec() -- retval = sub_info->retval;
      |
      |
call_usermodehelper_exec_work()
      |
      |
call_usermodehelper_exec_sync() -- sub_info->retval = ret;
      |
      | --> call_usermodehelper_exec_async() --> do_execve()
      |
kernel_wait4(pid, (int __user *)&ret, 0, NULL);

sub_info->retval is 256 after call kernel_wait4(), the function
call_usermodehelper_exec() returns sub_info->retval which is 256,
then call_modprobe() and __request_module() returns 256.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - update the commit message to explain the detailed reason

 kernel/kmod.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 3cd075c..5851444 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -28,6 +28,8 @@
 
 #include <trace/events/module.h>
 
+#define MODULE_NOT_FOUND 256
+
 /*
  * Assuming:
  *
@@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
+	if (strlen(module_name) == 0)
+		return MODULE_NOT_FOUND;
+
 	ret = security_kernel_module_request(module_name);
 	if (ret)
 		return ret;
-- 
2.1.0

[PATCH v2 4/4] test_kmod: Avoid potential double free in trigger_config_run_type()

[Tiezhu Yang]

Reset the member “test_fs” of the test configuration after a call
of the function “kfree_const” to a null pointer so that a double
memory release will not be performed.

Fixes: d9c6a72d6fa2 ("kmod: add test driver to stress test the module loader")
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - update the commit message suggested by Markus Elfring
  - add the Fixes tag

 lib/test_kmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index e651c37..eab5277 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -745,7 +745,7 @@ static int trigger_config_run_type(struct kmod_test_device *test_dev,
 		break;
 	case TEST_KMOD_FS_TYPE:
 		kfree_const(config->test_fs);
-		config->test_driver = NULL;
+		config->test_fs = NULL;
 		copied = config_copy_test_fs(config, test_str,
 					     strlen(test_str));
 		break;
-- 
2.1.0

Re: [PATCH v2 1/4] selftests: kmod: Use variable NAME in kmod_test_0001()

[Markus Elfring]

> Use the variable NAME instead of "\000" directly in kmod_test_0001().

Would this patch series have been a bit nicer together with
a cover letter?

Regards,
Markus

Re: [PATCH v2 1/4] selftests: kmod: Use variable NAME in kmod_test_0001()

[Tiezhu Yang]

On 04/20/2020 07:30 PM, Markus Elfring wrote:
>> Use the variable NAME instead of "\000" directly in kmod_test_0001().
> Would this patch series have been a bit nicer together with
> a cover letter?

OK, thanks for your suggestion. I will resend it with a cover letter.

Thanks,
Tiezhu Yang

>
> Regards,
> Markus

Re: [v2 1/4] selftests: kmod: Subsequent patch series with cover letters?

[Markus Elfring]

> OK, thanks for your suggestion.

Thanks for your acceptance.


> I will resend it with a cover letter.

I guess that such a resend would be needed only
if another version for this patch series will be published.
It seems that the attention can occasionally grow also for
the usage of helpful cover letters.

Regards,
Markus