linux-kernel - Re: [PATCH 2/2] kobject: use kvasprintf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20150914134205.a9d926185de33ddfd1b48808@linux-foundation.org>
Date:	Mon, 14 Sep 2015 13:42:05 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@...musvillemoes.dk> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/