On Fri, May 10, 2013 at 12:36 PM, Oleg Nesterov <o...@redhat.com> wrote:
> On 05/10, Lucas De Marchi wrote:
>>
>> Oh, right. Forgot about that. And this patch set should have been sent
>> as RFC, since I'm interested in feedback about the idea. What do you
>> think?
>
> Well, personally I think it would be better to use kasprintf(), see the
> patch I sent (it is actually wrong, needs kfree(args) before return).
>
> Or. How about the patch below? It should be split into 2 changes:
>
> 1. Introduce __argv_split(). It can have more callers, for
> example do_coredump() and ftrace_function_filter_re()
> can use it to avoid kstrndup() + kfree().
>
> 2. Change call_modprobe() to use kasprintf() + __argv_split().
Seems better. In your previous version I was troubled about
duplicating the string twice. Now it's weird freeing a
user-allocated-string,
but I think it's a good tradeoff and covers other use cases as you
pointed out as well.
>
> uncompiled/untested.
Ok. I'll give it a try.
>
> Oleg.
>
> --- x/lib/argv_split.c
> +++ x/lib/argv_split.c
> @@ -39,31 +39,15 @@ void argv_free(char **argv)
> }
> EXPORT_SYMBOL(argv_free);
>
> -/**
> - * argv_split - split a string at whitespace, returning an argv
> - * @gfp: the GFP mask used to allocate memory
> - * @str: the string to be split
> - * @argcp: returned argument count
> - *
> - * Returns an array of pointers to strings which are split out from
> - * @str. This is performed by strictly splitting on white-space; no
> - * quote processing is performed. Multiple whitespace characters are
> - * considered to be a single argument separator. The returned array
> - * is always NULL-terminated. Returns NULL on memory allocation
> - * failure.
> - *
> - * The source string at `str' may be undergoing concurrent alteration via
> - * userspace sysctl activity (at least). The argv_split() implementation
> - * attempts to handle this gracefully by taking a local copy to work on.
> +/*
> + * @argv_str should be kmalloc'ed by the caller, freed by this func.
> */
> -char **argv_split(gfp_t gfp, const char *str, int *argcp)
> +char **__argv_split(gfp_t gfp, const char *argv_str, int *argcp)
> {
> - char *argv_str;
> bool was_space;
> char **argv, **argv_ret;
> int argc;
>
> - argv_str = kstrndup(str, KMALLOC_MAX_SIZE - 1, gfp);
> if (!argv_str)
> return NULL;
>
> @@ -91,4 +75,28 @@ char **argv_split(gfp_t gfp, const char
> *argcp = argc;
> return argv_ret;
> }
> +
> +/**
> + * argv_split - split a string at whitespace, returning an argv
> + * @gfp: the GFP mask used to allocate memory
> + * @str: the string to be split
> + * @argcp: returned argument count
> + *
> + * Returns an array of pointers to strings which are split out from
> + * @str. This is performed by strictly splitting on white-space; no
> + * quote processing is performed. Multiple whitespace characters are
> + * considered to be a single argument separator. The returned array
> + * is always NULL-terminated. Returns NULL on memory allocation
> + * failure.
> + *
> + * The source string at `str' may be undergoing concurrent alteration via
> + * userspace sysctl activity (at least). The argv_split() implementation
> + * attempts to handle this gracefully by taking a local copy to work on.
> + */
> +char **argv_split(gfp_t gfp, const char *str, int *argcp)
> +{
> + char *dup = kstrndup(str, KMALLOC_MAX_SIZE - 1, gfp);
> + return __argv_split(gfp, dup, argcp);
> +}
> +
> EXPORT_SYMBOL(argv_split);
> --- x/kernel/kmod.c
> +++ x/kernel/kmod.c
> @@ -64,20 +64,16 @@ static DECLARE_RWSEM(umhelper_sem);
>
> #ifdef CONFIG_MODULES
>
> -/*
> - modprobe_path is set via /proc/sys.
> -*/
> -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> +/* modprobe_path is set via /proc/sys/kernel/modprobe */
> +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
>
> static void free_modprobe_argv(struct subprocess_info *info)
> {
> - kfree(info->argv[3]); /* check call_modprobe() */
> - kfree(info->argv);
> + argv_free(info->argv);
> }
>
> static int call_modprobe(char *module_name, int wait)
> {
> - struct subprocess_info *info;
> static char *envp[] = {
> "HOME=/",
> "TERM=linux",
> @@ -85,31 +81,24 @@ static int call_modprobe(char *module_na
> NULL
> };
>
> - char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
> + struct subprocess_info *info;
> + char **argv;
> + char *args;
> +
> + args = kasprintf(GFP_KERNEL, "%s %s", modprobe_path, module_name);
> + argv = __argv_split(GFP_KERNEL, args, NULL);
> if (!argv)
> goto out;
>
> - module_name = kstrdup(module_name, GFP_KERNEL);
> - if (!module_name)
> - goto free_argv;
> -
> - argv[0] = modprobe_path;
> - argv[1] = "-q";
> - argv[2] = "--";
> - argv[3] = module_name; /* check free_modprobe_argv() */
> - argv[4] = NULL;
> -
> info = call_usermodehelper_setup(modprobe_path, argv, envp,
> GFP_KERNEL,
> NULL, free_modprobe_argv, NULL);
> if (!info)
> - goto free_module_name;
> + goto free_argv;
>
> return call_usermodehelper_exec(info, wait | UMH_KILLABLE);
>
> -free_module_name:
> - kfree(module_name);
> free_argv:
> - kfree(argv);
> + argv_free(argv);
> out:
> return -ENOMEM;
> }
>
--
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
