Add an argument allowing argv_split to leave room for parameters to be
filled by the caller. This is useful in situations we want to split the
command and add a options as the last arguments.
Signed-off-by: Lucas De Marchi <[email protected]>
---
fs/coredump.c | 2 +-
include/linux/string.h | 3 ++-
kernel/sys.c | 2 +-
kernel/trace/trace_events_filter.c | 2 +-
kernel/trace/trace_probe.c | 2 +-
lib/argv_split.c | 13 +++++++------
6 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index d52f6bd..08697cf 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -565,7 +565,7 @@ void do_coredump(siginfo_t *siginfo)
goto fail_dropcount;
}
- helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
+ helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL, 0);
if (!helper_argv) {
printk(KERN_WARNING "%s failed to allocate memory\n",
__func__);
diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5..e2245e5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -116,7 +116,8 @@ extern char *kstrdup(const char *s, gfp_t gfp);
extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
-extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
+extern char **argv_split(gfp_t gfp, const char *str, int *argcp,
+ unsigned int extra);
extern void argv_free(char **argv);
extern bool sysfs_streq(const char *s1, const char *s2);
diff --git a/kernel/sys.c b/kernel/sys.c
index 0da73cf..d363325 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2196,7 +2196,7 @@ static int __orderly_poweroff(bool force)
};
int ret;
- argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL);
+ argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL, 0);
if (argv) {
ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
argv_free(argv);
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index e5b0ca8..3c107b2 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1981,7 +1981,7 @@ ftrace_function_filter_re(char *buf, int len, int *count)
while ((sep = strchr(str, ',')))
*sep = ' ';
- re = argv_split(GFP_KERNEL, str, count);
+ re = argv_split(GFP_KERNEL, str, count, 0);
kfree(str);
return re;
}
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 412e959..29128dd 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -770,7 +770,7 @@ int traceprobe_command(const char *buf, int (*createfn)(int, char **))
argc = 0;
ret = 0;
- argv = argv_split(GFP_KERNEL, buf, &argc);
+ argv = argv_split(GFP_KERNEL, buf, &argc, 0);
if (!argv)
return -ENOMEM;
diff --git a/lib/argv_split.c b/lib/argv_split.c
index e927ed0..9ebcbb0 100644
--- a/lib/argv_split.c
+++ b/lib/argv_split.c
@@ -43,20 +43,21 @@ 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
+ * @argcp: returned argument count without counting extras
+ * @extra: room left for extra arguments to be filled by caller
*
* 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.
+ * is always NULL-terminated, after any possible extra argument.
+ * 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 **argv_split(gfp_t gfp, const char *str, int *argcp, unsigned int extra)
{
char *argv_str;
bool was_space;
@@ -68,7 +69,7 @@ char **argv_split(gfp_t gfp, const char *str, int *argcp)
return NULL;
argc = count_argc(argv_str);
- argv = kmalloc(sizeof(*argv) * (argc + 2), gfp);
+ argv = kmalloc(sizeof(*argv) * (argc + 2 + extra), gfp);
if (!argv) {
kfree(argv_str);
return NULL;
@@ -85,7 +86,7 @@ char **argv_split(gfp_t gfp, const char *str, int *argcp)
*argv++ = argv_str;
}
}
- *argv = NULL;
+ argv[extra] = NULL;
if (argcp)
*argcp = argc;
--
1.8.2.2
Now that argv_split() leaves room for extra parameter, make
call_modprobe() use it.
Signed-off-by: Lucas De Marchi <[email protected]>
---
kernel/kmod.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 166aff5..70df90b 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -67,12 +67,12 @@ static DECLARE_RWSEM(umhelper_sem);
/*
modprobe_path is set via /proc/sys.
*/
-char modprobe_path[KMOD_PATH_LEN] = "/sbin/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)
@@ -84,8 +84,10 @@ static int call_modprobe(char *module_name, int wait)
"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
NULL
};
+ int argc;
+ char **argv;
- char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
+ argv = argv_split(GFP_KERNEL, modprobe_path, &argc, 1);
if (!argv)
goto out;
@@ -93,13 +95,8 @@ static int call_modprobe(char *module_name, int wait)
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,
+ argv[argc] = module_name; /* check free_modprobe_argv() */
+ info = call_usermodehelper_setup(argv[0], argv, envp, GFP_KERNEL,
NULL, free_modprobe_argv, NULL);
if (!info)
goto free_module_name;
@@ -109,7 +106,7 @@ static int call_modprobe(char *module_name, int wait)
free_module_name:
kfree(module_name);
free_argv:
- kfree(argv);
+ argv_free(argv);
out:
return -ENOMEM;
}
--
1.8.2.2
The hard-coded path of modprobe doesn't allow distros to put the binary
in another place without add links from one place to another. For
example, in recent versions of distros like Fedora, Arch and Suse, kmod
is the default tool to load modules and they have to create a link from
modprobe to kmod binary. kmod relies on argv[0] to know what to do but
in future they want to add commands like "kmod load -- modulename",
which is not possible if the path to the tool to load modules is
hard-coded in kernel.
Moreover, it's already possible to set the path through /proc/sys, but
it would have to be done very early in the boot sequence and on every
boot since modprobe may be called even before / is mounted. In this
scenario booting without and initrd would be more difficult as well.
Signed-off-by: Lucas De Marchi <[email protected]>
---
init/Kconfig | 7 +++++++
kernel/kmod.c | 2 +-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/init/Kconfig b/init/Kconfig
index 5341d72..72eee1c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1593,6 +1593,13 @@ menuconfig MODULES
if MODULES
+config DEFAULT_MODULE_LOAD_BIN
+ string "Default module load binary"
+ default "/sbin/modprobe -q --"
+ help
+ This option determines the default executable to be called when a
+ module is requested to be loaded via request_module().
+
config MODULE_FORCE_LOAD
bool "Forced module loading"
default n
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 70df90b..1828542 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -67,7 +67,7 @@ static DECLARE_RWSEM(umhelper_sem);
/*
modprobe_path is set via /proc/sys.
*/
-char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
+char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
static void free_modprobe_argv(struct subprocess_info *info)
{
--
1.8.2.2
On 05/10, Lucas De Marchi wrote:
>
> -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
> +char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
But even after 1/3 and 2/3 this can break free_modprobe_argv() ?
It assumes that argv[3] is module_name.
Oleg.
On Fri, May 10, 2013 at 9:58 AM, Oleg Nesterov <[email protected]> wrote:
>
> On 05/10, Lucas De Marchi wrote:
> >
> > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
> > +char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
>
> But even after 1/3 and 2/3 this can break free_modprobe_argv() ?
> It assumes that argv[3] is module_name.
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?
thanks
Lucas De Marchi
On 05/10, Oleg Nesterov wrote:
>
> On 05/10, Lucas De Marchi wrote:
> >
> > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
> > +char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
>
> But even after 1/3 and 2/3 this can break free_modprobe_argv() ?
> It assumes that argv[3] is module_name.
I'd suggest the patch below instead of 1 + 2. We can simply change
call_modprobe() to use kasprintf() + argv_split().
Oleg.
kmod.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
--- 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,29 @@ static int call_modprobe(char *module_na
NULL
};
- char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
- if (!argv)
- goto out;
+ struct subprocess_info *info;
+ char **argv;
+ char *args;
- module_name = kstrdup(module_name, GFP_KERNEL);
- if (!module_name)
- goto free_argv;
+ args = kasprintf(GFP_KERNEL, "%s %s", modprobe_path, module_name);
+ if (!args)
+ goto out;
- argv[0] = modprobe_path;
- argv[1] = "-q";
- argv[2] = "--";
- argv[3] = module_name; /* check free_modprobe_argv() */
- argv[4] = NULL;
+ argv = argv_split(GFP_KERNEL, args, NULL);
+ if (!argv)
+ goto free_args;
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);
+free_args:
+ kfree(args);
out:
return -ENOMEM;
}
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().
uncompiled/untested.
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;
}
On Fri, May 10, 2013 at 12:36 PM, Oleg Nesterov <[email protected]> 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
On 05/10, Lucas De Marchi wrote:
>
> On Fri, May 10, 2013 at 12:36 PM, Oleg Nesterov <[email protected]> wrote:
> > 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.
Oh, compared to other things we need to do this is nothing ;)
But to me it just looks better this way.
> Now it's weird freeing a
> user-allocated-string,
This is fine, the "weird" thing is that it frees the string even if
fails. But this simplifies the usage.
> but I think it's a good tradeoff and covers other use cases as you
> pointed out as well.
OK, good.
> Ok. I'll give it a try.
Please wait a bit, I'll send v2. See below.
> > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
No. This is incompatible change, we shouldn't do this.
> > + args = kasprintf(GFP_KERNEL, "%s %s", modprobe_path, module_name);
This should be kasprintf("%s -q -- %s").
And it needs a comment to explain that we are safe even if we race
with proc_dostring().
Oleg.
On 05/10, Oleg Nesterov wrote:
>
> On 05/10, Lucas De Marchi wrote:
> >
> > but I think it's a good tradeoff and covers other use cases as you
> > pointed out as well.
>
> OK, good.
Yes, perhaps this makes sense anyway but...
> > Ok. I'll give it a try.
>
> Please wait a bit, I'll send v2. See below.
Cough, wait ;)
Why do we need theese changes ????
> > > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> > > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
>
> No. This is incompatible change, we shouldn't do this.
Exactly. This can break a distro which writes to sys/kernel/modprobe.
And if we do not do this, you can simply make a single trivial patch
which does
- char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
+ char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
that it all. (or perhaps a kernel parameter makes more sense).
Yes, this doesn't allow to pass the additional arguments, but is it
that important?
Oleg.
On Fri, May 10, 2013 at 2:35 PM, Oleg Nesterov <[email protected]> wrote:
> On 05/10, Oleg Nesterov wrote:
>>
>> On 05/10, Lucas De Marchi wrote:
>> >
>> > but I think it's a good tradeoff and covers other use cases as you
>> > pointed out as well.
>>
>> OK, good.
>
> Yes, perhaps this makes sense anyway but...
>
>> > Ok. I'll give it a try.
>>
>> Please wait a bit, I'll send v2. See below.
>
> Cough, wait ;)
>
> Why do we need theese changes ????
>
>> > > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
>> > > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
>>
>> No. This is incompatible change, we shouldn't do this.
>
> Exactly. This can break a distro which writes to sys/kernel/modprobe.
>
> And if we do not do this, you can simply make a single trivial patch
> which does
>
> - char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> + char modprobe_path[KMOD_PATH_LEN] = CONFIG_DEFAULT_MODULE_LOAD_BIN;
>
> that it all. (or perhaps a kernel parameter makes more sense).
>
> Yes, this doesn't allow to pass the additional arguments, but is it
> that important?
Yes, because I don't want to simply change the binary to use, I want
to be able to use a general "kmod" binary that accept a command like
"load". Next version of kmod will accept things like this (see the
commit message in patch 3/3):
kmod load modulename
oh... and busybox users might be interested in this as well since they
can do "busybox modprobe modulename" directly, too
Lucas De Marchi
On Fri, May 10, 2013 at 2:10 PM, Oleg Nesterov <[email protected]> wrote:
> On 05/10, Lucas De Marchi wrote:
>>
>> On Fri, May 10, 2013 at 12:36 PM, Oleg Nesterov <[email protected]> wrote:
>> > 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.
>
> Oh, compared to other things we need to do this is nothing ;)
>
> But to me it just looks better this way.
>
>> Now it's weird freeing a
>> user-allocated-string,
>
> This is fine, the "weird" thing is that it frees the string even if
> fails. But this simplifies the usage.
>
>> but I think it's a good tradeoff and covers other use cases as you
>> pointed out as well.
>
> OK, good.
>
>> Ok. I'll give it a try.
>
> Please wait a bit, I'll send v2. See below.
>
>> > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
>> > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
>
> No. This is incompatible change, we shouldn't do this.
But then what option do we have? I don't think any distro writing to
sysctl would stop working without "-q --". And if they are indeed
using this during boot, I think they would be much safer to just
change to set this at compile time like this patch is trying to do.
Otherwise, the options are really ugly:
1) always give "-q --" by putting this on the kasprintf() call:
kasprintf(GFP_KERNEL, "%s -q -- %s", modprobe_command, module_name).
2) provide a proc_dostring_modprobe, which ensures "-q --" is appended
to the provided path
(2) is ugly as (1), but would apply just to not break current setups
and if set in Kconfig it's "-q --"-free.
>
>> > + args = kasprintf(GFP_KERNEL, "%s %s", modprobe_path, module_name);
>
> This should be kasprintf("%s -q -- %s").
>
> And it needs a comment to explain that we are safe even if we race
> with proc_dostring().
ok
Lucas De Marchi
On 05/10, Lucas De Marchi wrote:
>
> On Fri, May 10, 2013 at 2:35 PM, Oleg Nesterov <[email protected]> wrote:
> > On 05/10, Oleg Nesterov wrote:
> >>
> >> > > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> >> > > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
> >>
> >> No. This is incompatible change, we shouldn't do this.
> >
> > Exactly. This can break a distro which writes to sys/kernel/modprobe.
> >
> > And if we do not do this, you can simply make a single trivial patch
> > which does
...
> > that it all. (or perhaps a kernel parameter makes more sense).
> >
> > Yes, this doesn't allow to pass the additional arguments, but is it
> > that important?
>
> Yes, because I don't want to simply change the binary to use, I want
> to be able to use a general "kmod" binary that accept a command like
> "load". Next version of kmod will accept things like this (see the
> commit message in patch 3/3):
Well, but a link to the binary which checks argv[0] or a trivial executable
which simply execs "kmod load" looks like the simple workaround. And this
doesn't need to recompile the kernel.
Lucas, I simply do not know...
Andrew, Rusty, what do you think? Can we do the change above? Do we
really want CONFIG_MODPROBE_PATH or a kernel parameter ?
Oleg.
On 05/11, Lucas De Marchi wrote:
>
> On Fri, May 10, 2013 at 2:10 PM, Oleg Nesterov <[email protected]> wrote:
> >
> >> > -char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
> >> > +char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe -q --";
> >
> > No. This is incompatible change, we shouldn't do this.
>
> But then what option do we have? I don't think any distro writing to
> sysctl would stop working without "-q --".
Who knows?
Again, I won't argue, I simply do not know. But this is incompatible
change.
> And if they are indeed
> using this during boot, I think they would be much safer to just
> change to set this at compile time like this patch is trying to do.
Personally I think that "compile time" is not the best choice, but
I won't argue.
> Otherwise, the options are really ugly:
>
> 1) always give "-q --" by putting this on the kasprintf() call:
> kasprintf(GFP_KERNEL, "%s -q -- %s", modprobe_command, module_name).
> 2) provide a proc_dostring_modprobe, which ensures "-q --" is appended
> to the provided path
Agreed, both options do not look nice.
Lucas. I will be happy to resend the argv_split/call_modprobe changes,
but I can't judge whether we can/want do this.
Oleg.
On 05/13, Oleg Nesterov wrote:
>
> Lucas. I will be happy to resend the argv_split/call_modprobe changes,
speaking of argv_split...
What do you all think about the hack below? Sure, this is user-visible
and incompatible change, but perhaps we can do this?
Let me quote Lennart:
Currently, if the kernel generates one command line string from the core
pattern (if it is one with a |) and then splits that up again where it
finds spaces. This is really broken for %e if a process name contains
spaces.
Yes, we can change format_corename() to construct "argv" by hand, and
this was my iniital plan. But perhaps it would be better to not uglify
this code even more?
With the patch below we can trivially fix the problem,
+ char *fmt = ispipe ? "\e%s\e" : "%s";
...
- err = cn_printf(cn, "%s", current->comm);
+ err = cn_printf(cn, fmt, current->comm);
Or this ESC hack is too ugly or can break something?
Oleg.
------------------------------------------------------------------------------
[PATCH] teach argv_split() to ignore the spaces surrounded by \e
This patch assumes that nobody ever wants "\e" in the string
passed to argv_split().
With this change argv_split() treats '\e' as white-space and
ignores all other spaces till the next '\e'. For example,
argv("1\e2 3\e \e 4 \e") returns
[0] = "1",
[1] = "2 3",
[2] = " 4 ",
[3] = NULL,
This allows us to trivially fix format_corename(), we do not
want to split, say, current->comm if it has spaces.
Signed-off-by: Oleg Nesterov <[email protected]>
---
lib/argv_split.c | 37 +++++++++++++++++++++++++------------
1 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/lib/argv_split.c b/lib/argv_split.c
index e927ed0..e74221c 100644
--- a/lib/argv_split.c
+++ b/lib/argv_split.c
@@ -8,13 +8,23 @@
#include <linux/slab.h>
#include <linux/export.h>
+static bool argv_isspace(char ch, bool *in_esc)
+{
+ if (unlikely(ch == '\e')) {
+ *in_esc = !*in_esc;
+ return true;
+ }
+
+ return !*in_esc && isspace(ch);
+}
+
static int count_argc(const char *str)
{
+ bool was_space, in_esc;
int count = 0;
- bool was_space;
- for (was_space = true; *str; str++) {
- if (isspace(*str)) {
+ for (in_esc = false, was_space = true; *str; str++) {
+ if (argv_isspace(*str, &in_esc)) {
was_space = true;
} else if (was_space) {
was_space = false;
@@ -45,21 +55,24 @@ EXPORT_SYMBOL(argv_free);
* @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.
+ * Returns an array of pointers to strings which are split out from @str.
+ * The returned array is always NULL-terminated. Returns NULL on memory
+ * allocation failure.
+ *
+ * This is performed by splitting on white-space, no quote processing is
+ * performed except: '\e' is counted as space, and all spaces till the
+ * next '\e' are ignored. Multiple whitespace characters are considered
+ * to be a single argument separator.
*
* 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 *argv_str;
- bool was_space;
+ bool was_space, in_esc;
char **argv, **argv_ret;
int argc;
@@ -76,8 +89,8 @@ char **argv_split(gfp_t gfp, const char *str, int *argcp)
*argv = argv_str;
argv_ret = ++argv;
- for (was_space = true; *argv_str; argv_str++) {
- if (isspace(*argv_str)) {
+ for (in_esc = false, was_space = true; *argv_str; argv_str++) {
+ if (argv_isspace(*argv_str, &in_esc)) {
was_space = true;
*argv_str = 0;
} else if (was_space) {
--
1.5.5.1
On Mon, 2013-05-13 at 16:35 +0200, Oleg Nesterov wrote:
> Yes, we can change format_corename() to construct "argv" by hand, and
> this was my iniital plan. But perhaps it would be better to not uglify
> this code even more?
Sure this \e is less code, but it seems pretty ugly to me. Maybe a way
to keep fs/coredump.c sane would be always constructing an argv, and
then in the !ispipe case just join them into one string.
Though I'm still inclined to change systemd to read /proc/pid/cmdline
like abrt does; that way it works on current kernels too.
For what it's worth I noticed this problem with dconf, which uses
g_thread_new ("dconf worker", ...), and g_thread_new uses prctl
(PR_SET_NAME).
On 05/13, Colin Walters wrote:
>
> On Mon, 2013-05-13 at 16:35 +0200, Oleg Nesterov wrote:
>
> > Yes, we can change format_corename() to construct "argv" by hand, and
> > this was my iniital plan. But perhaps it would be better to not uglify
> > this code even more?
>
> Sure this \e is less code, but it seems pretty ugly to me.
Yes, I am not proud of this idea. But it is simple.
> Maybe a way
> to keep fs/coredump.c sane would be always constructing an argv, and
> then in the !ispipe case just join them into one string.
I don't think we should construct an argv if !ispipe, but this is minor.
The patch should be simple anyway. Just I do not want to touch this code ;)
and to complicate it more. and create another (2nd) case when when we need
to construct argv by hand.
> Though I'm still inclined to change systemd to read /proc/pid/cmdline
> like abrt does; that way it works on current kernels too.
Oh, I will be really happy to leave this this code alone and do nothing ;)
except format_corename() has another bug, it can leak ->corename but this
is another story (I'll send the patch).
Oleg.