Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

[Andrew Morton]

On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:

> Sometimes kobject_set_name_vargs is called with a format string
> conaining no %, or a format string of precisely "%s", where the single
> vararg happens to point to .rodata. kvasprintf_const detects these
> cases for us and returns a copy of that pointer instead of duplicating
> the string, thus saving some run-time memory. Otherwise, it falls back
> to kvasprintf. We just need to always deallocate ->name using
> kfree_const.
> 
> Unfortunately, the dance we need to do to perform the '/' -> '!'
> sanitization makes the resulting code rather ugly.
> 
> I instrumented kstrdup_const to provide some statistics on the memory
> saved, and for me this gave an additional ~14KB after boot (306KB was
> already saved; this patch bumped that to 320KB). I have
> KMALLOC_SHIFT_LOW==3, and since 80% of the kvasprintf_const hits were
> satisfied by an 8-byte allocation, the 14K would roughly be quadrupled
> when KMALLOC_SHIFT_LOW==5. Whether these numbers are sufficient to
> justify the ugliness I'll leave to others to decide.

Do we have other callsites whcih can benefit from switching to
kvasprintf_const()?  The [1/2] changelog didn't make this clear.

> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3e3a5c3cb330..fee2fd950306 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -257,18 +257,32 @@ static int kobject_add_internal(struct kobject *kobj)
>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>  				  va_list vargs)
>  {
> -	char *s;
> +	const char *s;
>  
>  	if (kobj->name && !fmt)
>  		return 0;
>  
> -	s = kvasprintf(GFP_KERNEL, fmt, vargs);
> +	s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
>  	if (!s)
>  		return -ENOMEM;
>  
> -	/* ewww... some of these buggers have '/' in the name ... */
> -	strreplace(s, '/', '!');
> -	kfree(kobj->name);
> +	/*
> +	 * ewww... some of these buggers have '/' in the name ... If
> +	 * that's the case, we need to make sure we have an actual
> +	 * allocated copy to modify, since kvasprintf_const may have
> +	 * returned something from .rodata.
> +	 */
> +	if (strchr(s, '/')) {

It doesn't look too ugly to me.

Can we test here whether kvasprintf_const() really returned somethnig
in .rodata?

> +		char *t;
> +
> +		t = kstrdup(s, GFP_KERNEL);
> +		kfree_const(s);
> +		if (!t)
> +			return -ENOMEM;
> +		strreplace(t, '/', '!');
> +		s = t;
> +	}
> +	kfree_const(kobj->name);
>  	kobj->name = s;
>  
>  	return 0;

--
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/