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

[Tiezhu Yang]

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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > ---
> > > > >    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