| Message ID | 1587386035-5188-4-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some issues about kmod | expand |
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
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
+++ 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
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
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
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
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;
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(+)