[PATCH] usermodehelper: kill the sub_info->path[0] check

[PATCH] usermodehelper: kill the sub_info->path[0] check

[Oleg Nesterov]

call_usermodehelper_exec() does nothing but returns success if
path[0] == 0. The only user which needs this strange feature is
request_module(), it can check modprobe_path[0] itself like other
users do if they want to detect the "disabled by admin" case.

Kill it. Not only it looks strange, it can confuse other callers.
And this allows us to revert 264b83c0 "usermodehelper: check
subprocess_info->path != NULL", do_execve(NULL) is safe.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 8241906..fb32636 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -147,6 +147,9 @@ int __request_module(bool wait, const char *fmt, ...)
 	 */
 	WARN_ON_ONCE(wait && current_is_async());
 
+	if (!modprobe_path[0])
+		return 0;
+
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
 	va_end(args);
@@ -569,14 +572,6 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	int retval = 0;
 
 	helper_lock();
-	if (!sub_info->path) {
-		retval = -EINVAL;
-		goto out;
-	}
-
-	if (sub_info->path[0] == '\0')
-		goto out;
-
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
 		goto out;
-- 
1.5.5.1

Re: [PATCH] usermodehelper: kill the sub_info->path[0] check

[Rusty Russell]

Oleg Nesterov <oleg@redhat.com> writes:
> call_usermodehelper_exec() does nothing but returns success if
> path[0] == 0. The only user which needs this strange feature is
> request_module(), it can check modprobe_path[0] itself like other
> users do if they want to detect the "disabled by admin" case.
>
> Kill it. Not only it looks strange, it can confuse other callers.
> And this allows us to revert 264b83c0 "usermodehelper: check
> subprocess_info->path != NULL", do_execve(NULL) is safe.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Nice cleanup.  It was definitely weird...

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks!
Rusty.

> ---
>  kernel/kmod.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 8241906..fb32636 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -147,6 +147,9 @@ int __request_module(bool wait, const char *fmt, ...)
>  	 */
>  	WARN_ON_ONCE(wait && current_is_async());
>  
> +	if (!modprobe_path[0])
> +		return 0;
> +
>  	va_start(args, fmt);
>  	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
>  	va_end(args);
> @@ -569,14 +572,6 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	int retval = 0;
>  
>  	helper_lock();
> -	if (!sub_info->path) {
> -		retval = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (sub_info->path[0] == '\0')
> -		goto out;
> -
>  	if (!khelper_wq || usermodehelper_disabled) {
>  		retval = -EBUSY;
>  		goto out;
> -- 
> 1.5.5.1

Re: [PATCH] usermodehelper: kill the sub_info->path[0] check

[Lucas De Marchi]

Hi Oleg,

On Mon, May 20, 2013 at 1:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> call_usermodehelper_exec() does nothing but returns success if
> path[0] == 0. The only user which needs this strange feature is
> request_module(), it can check modprobe_path[0] itself like other
> users do if they want to detect the "disabled by admin" case.
>
> Kill it. Not only it looks strange, it can confuse other callers.
> And this allows us to revert 264b83c0 "usermodehelper: check
> subprocess_info->path != NULL", do_execve(NULL) is safe.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-By: Lucas De Marchi <lucas.demarchi@intel.com>

But... see below.

> ---
>  kernel/kmod.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 8241906..fb32636 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -147,6 +147,9 @@ int __request_module(bool wait, const char *fmt, ...)
>          */
>         WARN_ON_ONCE(wait && current_is_async());
>
> +       if (!modprobe_path[0])
> +               return 0;
> +

Any reason to not return -EINVAL here except for maintaining the
previous behavior? Checking the callers reveals just a few of them
actually check the return value and IMO this is no different than the
binary not existing and failing later on exec.

>         va_start(args, fmt);
>         ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
>         va_end(args);
> @@ -569,14 +572,6 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>         int retval = 0;
>
>         helper_lock();
> -       if (!sub_info->path) {
> -               retval = -EINVAL;
> -               goto out;
> -       }
> -
> -       if (sub_info->path[0] == '\0')
> -               goto out;
> -
>         if (!khelper_wq || usermodehelper_disabled) {
>                 retval = -EBUSY;
>                 goto out;
> --

Lucas De Marchi

Re: [PATCH] usermodehelper: kill the sub_info->path[0] check

[Oleg Nesterov]

Hi Lucas,

On 05/21, Lucas De Marchi wrote:
>
> Acked-By: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks.

> > @@ -147,6 +147,9 @@ int __request_module(bool wait, const char *fmt, ...)
> >          */
> >         WARN_ON_ONCE(wait && current_is_async());
> >
> > +       if (!modprobe_path[0])
> > +               return 0;
> > +
>
> Any reason to not return -EINVAL here except for maintaining the
> previous behavior?

But for what?

Keep the previous behaviour is important. And this matches, say,
kobject_uevent_env().

> Checking the callers reveals just a few of them
> actually check the return value and IMO this is no different than the
> binary not existing and failing later on exec.

Yes, agreed. And perhaps request_module() is different. For example,
search_binary_handler(). Perhaps we should change this, but imho this
needs more patches/discussion.

This is like the previous commit 264b83c0 reverted by this patch, the
change tries to be simple and conservative.

Oleg.