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 <[email protected]>
---
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
Oleg Nesterov <[email protected]> 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 <[email protected]>
Nice cleanup. It was definitely weird...
Acked-by: Rusty Russell <[email protected]>
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
Hi Oleg,
On Mon, May 20, 2013 at 1:55 PM, Oleg Nesterov <[email protected]> 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 <[email protected]>
Acked-By: Lucas De Marchi <[email protected]>
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
Hi Lucas,
On 05/21, Lucas De Marchi wrote:
>
> Acked-By: Lucas De Marchi <[email protected]>
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.