Re: [PATCH 1/2] teach argv_split() to handle the mutable strings

[Andrew Morton <akpm@linux-foundation.org>]

On Sat, 16 Mar 2013 21:23:53 +0100 Oleg Nesterov <oleg@redhat.com> wrote:

> argv_split() allocates argv[count_argc(str)] array and assumes that
> it will find the same number of arguments later. This is obviously
> wrong if this string can be changed, say, by sysctl.
> 
> With this patch argv_split() kstrndup's the whole string and does
> not split it, we simply replace the spaces with zeroes and keep the
> allocated memory in argv[-1] for argv_free(arg).
> 
> We do not use argv[0] because:
> 
> 	- str can be all-spaces or empty. In fact this case is fine,
> 	  we could kfree() it before return, but:
> 
> 	- str can have a space at the start, and we can not rely on
> 	  kstrndup(skip_arg(str)) because it can equally race if this
> 	  string is mutable.
> 
> Also, simplify count_argc() and kill the no longer used skip_arg().
> 
> ...
>
>  char **argv_split(gfp_t gfp, const char *str, int *argcp)
>  {
> -	int argc = count_argc(str);
> -	char **argv = kzalloc(sizeof(*argv) * (argc+1), gfp);
> -	char **argvp;
> -
> -	if (argv == NULL)
> -		goto out;
> -
> -	if (argcp)
> -		*argcp = argc;
> -
> -	argvp = argv;
> -
> -	while (*str) {
> -		str = skip_spaces(str);
> -
> -		if (*str) {
> -			const char *p = str;
> -			char *t;
> -
> -			str = skip_arg(str);
> +	char *argv_str;
> +	bool was_space;
> +	char **argv, **argv_ret;
> +	int argc;
> +
> +	argv_str = kstrndup(str, KMALLOC_MAX_SIZE, gfp);

hm, I think that works.

kstrndup() does kmalloc_track_caller(len+1, gfp) so your
KMALLOC_MAX_SIZE is off-by-one?


>From reading the code it is rather unobvious why things were
implemented in this fashion.  People may come along in five years and
"clean it up".  Hence we should explain, no?

--- a/lib/argv_split.c~argv_split-teach-it-to-handle-mutable-strings-fix
+++ a/lib/argv_split.c
@@ -51,6 +51,10 @@ EXPORT_SYMBOL(argv_free);
  * 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)
 {
_