lkml.org 
[lkml]   [2020]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/4] kmod: Return directly if module name is empty in request_module()
From
Date
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
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

\
 
 \ /
  Last update: 2020-04-20 06:09    [W:0.095 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site