linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Fix some issues about kmod
@ 2020-04-20 12:33 Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 1/4] selftests: kmod: Use variable NAME in kmod_test_0001() Tiezhu Yang
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-20 12:33 UTC (permalink / raw)
  To: Luis Chamberlain, Shuah Khan, Andrew Morton, Jessica Yu
  Cc: linux-kselftest, linux-kernel, Markus Elfring, Xuefeng Li

v3:
  - use the quotes with correct format in the commit message of patch 4/4,
    sorry for that

Tiezhu Yang (4):
  selftests: kmod: Use variable NAME in kmod_test_0001()
  kmod: Remove redundant "be an" in the comment
  kmod: Return directly if module name is empty in request_module()
  test_kmod: Avoid potential double free in trigger_config_run_type()

 kernel/kmod.c                        | 10 +++++++---
 lib/test_kmod.c                      |  2 +-
 tools/testing/selftests/kmod/kmod.sh |  4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/4] selftests: kmod: Use variable NAME in kmod_test_0001()
  2020-04-20 12:33 [PATCH v3 0/4] Fix some issues about kmod Tiezhu Yang
@ 2020-04-20 12:33 ` Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 2/4] kmod: Remove redundant "be an" in the comment Tiezhu Yang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-20 12:33 UTC (permalink / raw)
  To: Luis Chamberlain, Shuah Khan, Andrew Morton, Jessica Yu
  Cc: linux-kselftest, linux-kernel, Markus Elfring, Xuefeng Li

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>
---

v3:
  - no changes

v2:
  - just add the 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/4] kmod: Remove redundant "be an" in the comment
  2020-04-20 12:33 [PATCH v3 0/4] Fix some issues about kmod Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 1/4] selftests: kmod: Use variable NAME in kmod_test_0001() Tiezhu Yang
@ 2020-04-20 12:33 ` Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module() Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 4/4] test_kmod: Avoid potential double free in trigger_config_run_type() Tiezhu Yang
  3 siblings, 0 replies; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-20 12:33 UTC (permalink / raw)
  To: Luis Chamberlain, Shuah Khan, Andrew Morton, Jessica Yu
  Cc: linux-kselftest, linux-kernel, Markus Elfring, Xuefeng Li

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>
---

v3:
  - no changes

v2:
  - just add the 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-20 12:33 [PATCH v3 0/4] Fix some issues about kmod Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 1/4] selftests: kmod: Use variable NAME in kmod_test_0001() Tiezhu Yang
  2020-04-20 12:33 ` [PATCH v3 2/4] kmod: Remove redundant "be an" in the comment Tiezhu Yang
@ 2020-04-20 12:33 ` Tiezhu Yang
  2020-04-20 18:19   ` Luis Chamberlain
  2020-04-20 12:33 ` [PATCH v3 4/4] test_kmod: Avoid potential double free in trigger_config_run_type() Tiezhu Yang
  3 siblings, 1 reply; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-20 12:33 UTC (permalink / raw)
  To: Luis Chamberlain, Shuah Khan, Andrew Morton, Jessica Yu
  Cc: linux-kselftest, linux-kernel, Markus Elfring, Xuefeng Li

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>
---

v3:
  - no changes

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 4/4] test_kmod: Avoid potential double free in trigger_config_run_type()
  2020-04-20 12:33 [PATCH v3 0/4] Fix some issues about kmod Tiezhu Yang
                   ` (2 preceding siblings ...)
  2020-04-20 12:33 ` [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module() Tiezhu Yang
@ 2020-04-20 12:33 ` Tiezhu Yang
  3 siblings, 0 replies; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-20 12:33 UTC (permalink / raw)
  To: Luis Chamberlain, Shuah Khan, Andrew Morton, Jessica Yu
  Cc: linux-kselftest, linux-kernel, Markus Elfring, Xuefeng Li

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>
---

v3:
  - use the quotes with correct format in the commit message,
    sorry for that

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-20 12:33 ` [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module() Tiezhu Yang
@ 2020-04-20 18:19   ` Luis Chamberlain
  2020-04-21  3:07     ` Tiezhu Yang
  0 siblings, 1 reply; 11+ messages in thread
From: Luis Chamberlain @ 2020-04-20 18:19 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Shuah Khan, Andrew Morton, Jessica Yu, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li

On Mon, Apr 20, 2020 at 08:33:54PM +0800, Tiezhu Yang wrote:
> 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>

Thanks for looking into this. I still cannot find where
userspace it returns 256. Can you? If I run modprobe without
an argument I see 1 returned.

At least kmod [0] has a series of cmd helper structs, the one for modprobe
seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
can be converted to 255. It can also return EXIT_FAILURE or EXIT_SUCCESS
and /usr/include/stdlib.h defines these as 1 and 0 respectively.

https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/

  Luis

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-20 18:19   ` Luis Chamberlain
@ 2020-04-21  3:07     ` Tiezhu Yang
  2020-04-21 14:49       ` Jessica Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-21  3:07 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Shuah Khan, Andrew Morton, Jessica Yu, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li

On 04/21/2020 02:19 AM, Luis Chamberlain wrote:
> On Mon, Apr 20, 2020 at 08:33:54PM +0800, Tiezhu Yang wrote:
>> 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>
> Thanks for looking into this. I still cannot find where
> userspace it returns 256. Can you? If I run modprobe without
> an argument I see 1 returned.
>
> At least kmod [0] has a series of cmd helper structs, the one for modprobe
> seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
> can be converted to 255. It can also return EXIT_FAILURE or EXIT_SUCCESS
> and /usr/include/stdlib.h defines these as 1 and 0 respectively.
>
> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
>
>    Luis

Here is my understanding:

When build and execute the following application, we can see the exit 
status is 256.

$ ./system
modprobe: FATAL: Module  not found in directory 
/lib/modules/4.18.0-147.5.1.el8_1.x86_64
exit status = 256

$ ./execl
modprobe: FATAL: Module  not found in directory 
/lib/modules/4.18.0-147.5.1.el8_1.x86_64
exit status = 256

$ cat system.c
#include <stdio.h>
#include <stdlib.h>

int main()
{
     int status = 0;

     status = system("modprobe ''");
     printf("exit status = %d\n", status);

     return status;
}

$ cat execl.c
#include <sys/wait.h>
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>

int main()
{
     pid_t pid, w;
     int status;

     pid = fork();
     if (pid == -1) {
         perror("fork");
         exit(EXIT_FAILURE);
     }

     if (pid == 0) {
         execl("/bin/sh", "sh", "-c", "modprobe aaa", (char *) 0);
     } else {
         w = waitpid(pid, &status, 0);
         if (w == -1) {
             perror("waitpid");
             exit(EXIT_FAILURE);
         }

         printf("exit status = %d\n", status);

         exit(EXIT_SUCCESS);
     }

     return 0;
}

The exit status of child process is wrote to the address of variable 
"status"
after call waitpid()in the user space that correspond with 
kernel_wait4() [1]
in the kernel space.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/exit.c#n1576


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-21  3:07     ` Tiezhu Yang
@ 2020-04-21 14:49       ` Jessica Yu
  2020-04-22  8:31         ` Luis Chamberlain
  2020-04-22  8:55         ` Tiezhu Yang
  0 siblings, 2 replies; 11+ messages in thread
From: Jessica Yu @ 2020-04-21 14:49 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Luis Chamberlain, Shuah Khan, Andrew Morton, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li

+++ Tiezhu Yang [21/04/20 11:07 +0800]:
>On 04/21/2020 02:19 AM, Luis Chamberlain wrote:
>>On Mon, Apr 20, 2020 at 08:33:54PM +0800, Tiezhu Yang wrote:
>>>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>
>>Thanks for looking into this. I still cannot find where
>>userspace it returns 256. Can you? If I run modprobe without
>>an argument I see 1 returned.
>>
>>At least kmod [0] has a series of cmd helper structs, the one for modprobe
>>seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
>>can be converted to 255. It can also return EXIT_FAILURE or EXIT_SUCCESS
>>and /usr/include/stdlib.h defines these as 1 and 0 respectively.

I'm also seeing modprobe return 1 as exit status when I run it without
arguments. I don't think the 256 value is coming from modprobe though,
see below -

>>https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
>>
>>   Luis
>
>Here is my understanding:
>
>When build and execute the following application, we can see the exit 
>status is 256.
>
>$ ./system
>modprobe: FATAL: Module  not found in directory 
>/lib/modules/4.18.0-147.5.1.el8_1.x86_64
>exit status = 256
>
>$ ./execl
>modprobe: FATAL: Module  not found in directory 
>/lib/modules/4.18.0-147.5.1.el8_1.x86_64
>exit status = 256

I am going to guess this has something to do with how system() and
waitpid() (and the wait family of syscalls in general) encode the exit
status in their return values. According to their man pages, you need
to use the appropriate WIF* macros to get the actual exit code of the
child process.

From system(3):

    the return value is a "wait status" that can be examined using the
    macros described in waitpid(2).  (i.e., WIFEXITED(),
    WEXITSTATUS(), and so on)

From waitpid(2):

     If  wstatus  is  not  NULL,  wait()  and  waitpid() store status
     information in the int to which it points.  This integer can be
     inspected with the following macros (which take the integer
     itself as an argument, not a pointer to it, as is done in wait()
     and waitpid()!):

       WEXITSTATUS(wstatus)
              returns the exit status of the child.  This consists of
              the least significant 8 bits of the status argument that
              the child specified in a call to exit(3) or _exit(2) or
              as the argument for a return statement in main().  This
              macro should be employed only if WIFEXITED returned
              true.

In your test code, you are reading &status directly. To obtain the
exit status, you need to use WEXITSTATUS(status), or right shift the
value by 8 bits. That gives you 1, which was the original exit code
given by modprobe. That's why you see an exit code of 1 when running
modprobe directly and you see 256 when using system() and waitpid()
and don't use the WIF* macros.

As for why __request_module() returns 256, I am guessing this would
come from kernel_wait4(), but I did not dive into the call path to
verify this yet.

Jessica

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-21 14:49       ` Jessica Yu
@ 2020-04-22  8:31         ` Luis Chamberlain
  2020-04-22  8:55         ` Tiezhu Yang
  1 sibling, 0 replies; 11+ messages in thread
From: Luis Chamberlain @ 2020-04-22  8:31 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Tiezhu Yang, Shuah Khan, Andrew Morton, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li

On Tue, Apr 21, 2020 at 04:49:32PM +0200, Jessica Yu wrote:
> As for why __request_module() returns 256, I am guessing this would
> come from kernel_wait4(), but I did not dive into the call path to
> verify this yet.

I got it. I'll send a fix.

  Luis

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-21 14:49       ` Jessica Yu
  2020-04-22  8:31         ` Luis Chamberlain
@ 2020-04-22  8:55         ` Tiezhu Yang
  2020-04-22  9:01           ` Luis Chamberlain
  1 sibling, 1 reply; 11+ messages in thread
From: Tiezhu Yang @ 2020-04-22  8:55 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Luis Chamberlain, Shuah Khan, Andrew Morton, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li, Al Viro

On 04/21/2020 10:49 PM, Jessica Yu wrote:
> +++ Tiezhu Yang [21/04/20 11:07 +0800]:
>> On 04/21/2020 02:19 AM, Luis Chamberlain wrote:
>>> On Mon, Apr 20, 2020 at 08:33:54PM +0800, Tiezhu Yang wrote:
>>>> 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>
>>> Thanks for looking into this. I still cannot find where
>>> userspace it returns 256. Can you? If I run modprobe without
>>> an argument I see 1 returned.
>>>
>>> At least kmod [0] has a series of cmd helper structs, the one for 
>>> modprobe
>>> seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
>>> can be converted to 255. It can also return EXIT_FAILURE or 
>>> EXIT_SUCCESS
>>> and /usr/include/stdlib.h defines these as 1 and 0 respectively.
>
> I'm also seeing modprobe return 1 as exit status when I run it without
> arguments. I don't think the 256 value is coming from modprobe though,
> see below -
>
>>> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
>>>
>>>   Luis
>>
>> Here is my understanding:
>>
>> When build and execute the following application, we can see the exit 
>> status is 256.
>>
>> $ ./system
>> modprobe: FATAL: Module  not found in directory 
>> /lib/modules/4.18.0-147.5.1.el8_1.x86_64
>> exit status = 256
>>
>> $ ./execl
>> modprobe: FATAL: Module  not found in directory 
>> /lib/modules/4.18.0-147.5.1.el8_1.x86_64
>> exit status = 256
>
> I am going to guess this has something to do with how system() and
> waitpid() (and the wait family of syscalls in general) encode the exit
> status in their return values. According to their man pages, you need
> to use the appropriate WIF* macros to get the actual exit code of the
> child process.
>
> From system(3):
>
>    the return value is a "wait status" that can be examined using the
>    macros described in waitpid(2).  (i.e., WIFEXITED(),
>    WEXITSTATUS(), and so on)
>
> From waitpid(2):
>
>     If  wstatus  is  not  NULL,  wait()  and  waitpid() store status
>     information in the int to which it points.  This integer can be
>     inspected with the following macros (which take the integer
>     itself as an argument, not a pointer to it, as is done in wait()
>     and waitpid()!):
>
>       WEXITSTATUS(wstatus)
>              returns the exit status of the child.  This consists of
>              the least significant 8 bits of the status argument that
>              the child specified in a call to exit(3) or _exit(2) or
>              as the argument for a return statement in main(). This
>              macro should be employed only if WIFEXITED returned
>              true.
>
> In your test code, you are reading &status directly. To obtain the
> exit status, you need to use WEXITSTATUS(status), or right shift the
> value by 8 bits. That gives you 1, which was the original exit code
> given by modprobe. That's why you see an exit code of 1 when running
> modprobe directly and you see 256 when using system() and waitpid()
> and don't use the WIF* macros.
>
> As for why __request_module() returns 256, I am guessing this would
> come from kernel_wait4(), but I did not dive into the call path to
> verify this yet.

+Cc Al Viro <viro@zeniv.linux.org.uk>

Hi Al,

When module name is empty, __request_module() returns 256.
What do you think about this case and patch?
Thank you very much for your attention.

patch v3:
https://lore.kernel.org/patchwork/patch/1227274/

patch v4 (update the commit message):
https://lore.kernel.org/patchwork/patch/1227981/

>
> Jessica


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module()
  2020-04-22  8:55         ` Tiezhu Yang
@ 2020-04-22  9:01           ` Luis Chamberlain
  0 siblings, 0 replies; 11+ messages in thread
From: Luis Chamberlain @ 2020-04-22  9:01 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Jessica Yu, Shuah Khan, Andrew Morton, linux-kselftest,
	linux-kernel, Markus Elfring, Xuefeng Li, Al Viro

On Wed, Apr 22, 2020 at 04:55:34PM +0800, Tiezhu Yang wrote:
> On 04/21/2020 10:49 PM, Jessica Yu wrote:
> > +++ Tiezhu Yang [21/04/20 11:07 +0800]:
> > > On 04/21/2020 02:19 AM, Luis Chamberlain wrote:
> > > > On Mon, Apr 20, 2020 at 08:33:54PM +0800, Tiezhu Yang wrote:
> > > > > 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>
> > > > Thanks for looking into this. I still cannot find where
> > > > userspace it returns 256. Can you? If I run modprobe without
> > > > an argument I see 1 returned.
> > > > 
> > > > At least kmod [0] has a series of cmd helper structs, the one
> > > > for modprobe
> > > > seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
> > > > can be converted to 255. It can also return EXIT_FAILURE or
> > > > EXIT_SUCCESS
> > > > and /usr/include/stdlib.h defines these as 1 and 0 respectively.
> > 
> > I'm also seeing modprobe return 1 as exit status when I run it without
> > arguments. I don't think the 256 value is coming from modprobe though,
> > see below -
> > 
> > > > https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
> > > > 
> > > >   Luis
> > > 
> > > Here is my understanding:
> > > 
> > > When build and execute the following application, we can see the
> > > exit status is 256.
> > > 
> > > $ ./system
> > > modprobe: FATAL: Module  not found in directory
> > > /lib/modules/4.18.0-147.5.1.el8_1.x86_64
> > > exit status = 256
> > > 
> > > $ ./execl
> > > modprobe: FATAL: Module  not found in directory
> > > /lib/modules/4.18.0-147.5.1.el8_1.x86_64
> > > exit status = 256
> > 
> > I am going to guess this has something to do with how system() and
> > waitpid() (and the wait family of syscalls in general) encode the exit
> > status in their return values. According to their man pages, you need
> > to use the appropriate WIF* macros to get the actual exit code of the
> > child process.
> > 
> > From system(3):
> > 
> >    the return value is a "wait status" that can be examined using the
> >    macros described in waitpid(2).  (i.e., WIFEXITED(),
> >    WEXITSTATUS(), and so on)
> > 
> > From waitpid(2):
> > 
> >     If  wstatus  is  not  NULL,  wait()  and  waitpid() store status
> >     information in the int to which it points.  This integer can be
> >     inspected with the following macros (which take the integer
> >     itself as an argument, not a pointer to it, as is done in wait()
> >     and waitpid()!):
> > 
> >       WEXITSTATUS(wstatus)
> >              returns the exit status of the child.  This consists of
> >              the least significant 8 bits of the status argument that
> >              the child specified in a call to exit(3) or _exit(2) or
> >              as the argument for a return statement in main(). This
> >              macro should be employed only if WIFEXITED returned
> >              true.
> > 
> > In your test code, you are reading &status directly. To obtain the
> > exit status, you need to use WEXITSTATUS(status), or right shift the
> > value by 8 bits. That gives you 1, which was the original exit code
> > given by modprobe. That's why you see an exit code of 1 when running
> > modprobe directly and you see 256 when using system() and waitpid()
> > and don't use the WIF* macros.
> > 
> > As for why __request_module() returns 256, I am guessing this would
> > come from kernel_wait4(), but I did not dive into the call path to
> > verify this yet.
> 
> +Cc Al Viro <viro@zeniv.linux.org.uk>
> 
> Hi Al,
> 
> When module name is empty, __request_module() returns 256.
> What do you think about this case and patch?
> Thank you very much for your attention.

Its because of an old issue umh.c, I'll send a patch.

  Luis

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-04-22  9:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-20 12:33 [PATCH v3 0/4] Fix some issues about kmod Tiezhu Yang
2020-04-20 12:33 ` [PATCH v3 1/4] selftests: kmod: Use variable NAME in kmod_test_0001() Tiezhu Yang
2020-04-20 12:33 ` [PATCH v3 2/4] kmod: Remove redundant "be an" in the comment Tiezhu Yang
2020-04-20 12:33 ` [PATCH v3 3/4] kmod: Return directly if module name is empty in request_module() Tiezhu Yang
2020-04-20 18:19   ` Luis Chamberlain
2020-04-21  3:07     ` Tiezhu Yang
2020-04-21 14:49       ` Jessica Yu
2020-04-22  8:31         ` Luis Chamberlain
2020-04-22  8:55         ` Tiezhu Yang
2020-04-22  9:01           ` Luis Chamberlain
2020-04-20 12:33 ` [PATCH v3 4/4] test_kmod: Avoid potential double free in trigger_config_run_type() Tiezhu Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).