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

[Luis Chamberlain <mcgrof@kernel.org>]

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