Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> Hi Lucas,
>
> On 03/06, Lucas De Marchi wrote:
>>
>> On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/