Hi Oleg,
On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <[email protected]> wrote:
> On 02/25, Lucas De Marchi wrote:
>>
>> Yep. The current interface is confusing. I agree that a separate
>> setup() + exec() would make more sense.
>
> Great,
>
>> > @@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
>> > argv[3] = module_name; /* check free_modprobe_argv() */
>> > argv[4] = NULL;
>> >
>> > - return call_usermodehelper_fns(modprobe_path, argv, envp,
>> > - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> > + info = call_usermodehelper_setup(...); // better name, please...
>> > + if (!info)
>> > + goto free_modname;
>> > +
>> > + return call_usermodehelper_exec(info, wait);
>>
>> I'd say that in these cases the "call_" prefix has no meaning, and we
>> could just use a "usermodehelper" as the namespace.
>
> Oh, I agree with any naming.
>
> So, I hope you will send v2. I'd suggest to split the fixes. 1/3
> should create/export the new helpers, and 2-3 fix should call_modprobe()
> and call_usermodehelper_keys(). But this is up to you, I won't insist.
I was implementing this today, but looking into call_modprobe(), it is
never called with UMH_NO_WAIT. Doesn't it mean we can use a similar
trick as you used in __orderly_poweroff()? Then something similar to
this would be sufficient in this case (whitespace damaged):
diff --git a/kernel/kmod.c b/kernel/kmod.c
index b39f240..e4d6f6f 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -69,12 +69,6 @@ static DECLARE_RWSEM(umhelper_sem);
?*/
?char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
-static void free_modprobe_argv(struct subprocess_info *info)
-{
- kfree(info->argv[3]); /* check call_modprobe() */
- kfree(info->argv);
-}
-
?static int call_modprobe(char *module_name, int wait)
?{
? static char *envp[] = {
@@ -83,6 +77,7 @@ static int call_modprobe(char *module_name, int wait)
? "PATH=/sbin:/usr/sbin:/bin:/usr/bin",
? NULL
? };
+ int ret = -ENOMEM;
? char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
? if (!argv)
@@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
? argv[3] = module_name; /* check free_modprobe_argv() */
? argv[4] = NULL;
- return call_usermodehelper_fns(modprobe_path, argv, envp,
- wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+ ret = call_usermodehelper(modprobe_path, argv, envp,
+ ?wait | UMH_KILLABLE);
+ kfree(module_name);
?free_argv:
? kfree(argv);
?out:
- return -ENOMEM;
+ return ret;
?}
?/**
Lucas De Marchi
Hi Lucas,
On 03/06, Lucas De Marchi wrote:
>
> On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <[email protected]> wrote:
> >
> > So, I hope you will send v2. I'd suggest to split the fixes. 1/3
> > should create/export the new helpers, and 2-3 fix should call_modprobe()
> > and call_usermodehelper_keys(). But this is up to you, I won't insist.
>
> I was implementing this today, but looking into call_modprobe(), it is
> never called with UMH_NO_WAIT.
wait == T means UMH_WAIT_PROC, so we can't simply rely on CLONE_VFORK.
But probably we can rely on sys_wait4.
However,
> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
> ? argv[3] = module_name; /* check free_modprobe_argv() */
> ? argv[4] = NULL;
>
> - return call_usermodehelper_fns(modprobe_path, argv, envp,
> - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> + ret = call_usermodehelper(modprobe_path, argv, envp,
> + ?wait | UMH_KILLABLE);
> + kfree(module_name);
Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
we can return while the workqueue thread still tries to clone/exec/etc.
Oleg.
On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <[email protected]> wrote:
> Hi Lucas,
>
> On 03/06, Lucas De Marchi wrote:
>>
>> On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <[email protected]> wrote:
>> >
>> > So, I hope you will send v2. I'd suggest to split the fixes. 1/3
>> > should create/export the new helpers, and 2-3 fix should call_modprobe()
>> > and call_usermodehelper_keys(). But this is up to you, I won't insist.
>>
>> I was implementing this today, but looking into call_modprobe(), it is
>> never called with UMH_NO_WAIT.
>
> wait == T means UMH_WAIT_PROC, so we can't simply rely on CLONE_VFORK.
> But probably we can rely on sys_wait4.
yep, I was thinking about relying on sys_wait4.
>
> However,
>
>> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
>> argv[3] = module_name; /* check free_modprobe_argv() */
>> argv[4] = NULL;
>>
>> - return call_usermodehelper_fns(modprobe_path, argv, envp,
>> - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> + ret = call_usermodehelper(modprobe_path, argv, envp,
>> + wait | UMH_KILLABLE);
>> + kfree(module_name);
>
> Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
> and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
> we can return while the workqueue thread still tries to clone/exec/etc.
Even if it's killed, we would just free the resource we allocated
before. It would not be safe if we allocated in the init function and
freed in the cleanup. Or am I missing something?
thanks
Lucas De Marchi
On 03/07, Lucas De Marchi wrote:
>
> On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <[email protected]> wrote:
> >
> >> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
> >> argv[3] = module_name; /* check free_modprobe_argv() */
> >> argv[4] = NULL;
> >>
> >> - return call_usermodehelper_fns(modprobe_path, argv, envp,
> >> - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> >> + ret = call_usermodehelper(modprobe_path, argv, envp,
> >> + wait | UMH_KILLABLE);
> >> + kfree(module_name);
> >
> > Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
> > and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
> > we can return while the workqueue thread still tries to clone/exec/etc.
>
> Even if it's killed, we would just free the resource we allocated
> before.
Yes, and after that ____call_usermodehelper() can do
do_execve(module_name) ?
> It would not be safe if we allocated in the init function and
> freed in the cleanup.
But we do? We free this memory in cleanup ? And I was allocated by us.
sub_info itself can't go away (if you meant this), but
sub_info->path/argv/envp can.
> Or am I missing something?
Or me...
Oleg.
On Thu, Mar 7, 2013 at 5:07 PM, Oleg Nesterov <[email protected]> wrote:
> On 03/07, Lucas De Marchi wrote:
>>
>> On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <[email protected]> wrote:
>> >
>> >> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
>> >> argv[3] = module_name; /* check free_modprobe_argv() */
>> >> argv[4] = NULL;
>> >>
>> >> - return call_usermodehelper_fns(modprobe_path, argv, envp,
>> >> - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> >> + ret = call_usermodehelper(modprobe_path, argv, envp,
>> >> + wait | UMH_KILLABLE);
>> >> + kfree(module_name);
>> >
>> > Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
>> > and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
>> > we can return while the workqueue thread still tries to clone/exec/etc.
>>
>> Even if it's killed, we would just free the resource we allocated
>> before.
>
> Yes, and after that ____call_usermodehelper() can do
> do_execve(module_name) ?
>
>> It would not be safe if we allocated in the init function and
>> freed in the cleanup.
>
> But we do? We free this memory in cleanup ? And I was allocated by us.
>
> sub_info itself can't go away (if you meant this), but
> sub_info->path/argv/envp can.
Oh... you are right - the UMH_KILLABLE is the problem here. Dunno what
I was thinking :-/. I will fix my pending the patches.
thanks
Lucas De Marchi