Messages in this thread | | | Subject | Re: [PATCH 3/4] kmod: Return directly if module name is empty in request_module() | From | Tiezhu Yang <> | Date | Mon, 20 Apr 2020 12:11:21 +0800 |
| |
On 04/20/2020 12:08 PM, Tiezhu Yang wrote: > On 04/18/2020 03:19 PM, Luis Chamberlain wrote: >> On Sat, Apr 18, 2020 at 01:58:45PM +0800, Tiezhu Yang wrote: >>> On 04/18/2020 01:48 PM, Luis Chamberlain wrote: >>>> On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain >>>> <mcgrof@kernel.org> wrote: >>>>> On Sat, Apr 18, 2020 at 01:19:59PM +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. >>>>>> >>>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>>>>> --- >>>>>> 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; >>>>> I'd rather we just use something standard like -EINVAL. >>>>> What do we return if its not found? Then use that value. >>>> Also, are we testing for this condition yet? If not can we add one? >>> Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh >>> tests >>> this case and expects result MODULE_NOT_FOUND which is 256. >> OK I see now I had put: >> >> errno_name_to_val() >> { >> case "$1" in >> # kmod calls modprobe and upon of a module not found >> # modprobe returns just 1... However in the >> # kernel we *sometimes* see 256... >> MODULE_NOT_FOUND) >> echo 256;; >> >> I found that through testing, however there was nothing set in stone, >> nothing documented. While you are at it, can you find the places where >> this is returned in the kernel code? We should clear this up and >> se things straight. We cannot change what we gave userspace already >> though. > > Call Trace: > > __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); > > __request_module() returns the exist status of child process, if > module name
Sorry for the typo: exist status --> exit status
> is empty or non-exist, sub_info->retval is 256 after call kernel_wait4(). > > Should I add this analysis to the commit message? > > Thanks, > Tiezhu Yang > >> >> Luis >
| |