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] [day] [month] [year] [list]

Message-ID: <87lhc8wp85.fsf@rasmusvillemoes.dk>
Date:	Tue, 15 Sep 2015 01:33:30 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andrew Morton <akpm@...ux-foundation.org>
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 Mon, Sep 14 2015, Andrew Morton <akpm@...ux-foundation.org> wrote:

> On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
>
> Do we have other callsites whcih can benefit from switching to
> kvasprintf_const()?  The [1/2] changelog didn't make this clear.

There are a few, but kobject_set_name_vargs was the only I found that
would give KB savings (at least for my setup). Finding all places the
return value is freed and switch over to kfree_const is a little
work, and there's often also some struct member to change from "char*" to
"const char*" with a little fallout. IMHO, such constifications
would count as nice side effects, but I could also see why some might
think of it as useless churn.

A few candidates:

drivers/gpu/drm/drm_drv.c: drm_dev_set_unique()

drivers/xen/xenbus/xenbus_xs.c: xenbus_printf(): This one's easy, as the
kvasprintf return value is local to the function. But that also means we
wouldn't save any longterm memory. Many callers pass a format of "%d"
and then a literal 0 or 1, so they could be changed to passing "0" or
"1", saving a tiny bit of .text and a few cycles.

sound/pci/hda/hda_codec.c: snd_hda_codec_pcm_new(): Most callers pass
literals or "%s". I'm pretty sure the only kfree function to change is
the one in release_pcm a few lines above, so the biggest problem would
be changing struct hda_pcm->name to const char* and fixing the fallout
from that.

> It doesn't look too ugly to me.
>
> Can we test here whether kvasprintf_const() really returned somethnig
> in .rodata?

Yeah, I also thought about avoiding kstrdup() if we already have a
modifiable string. We'd have to move is_kernel_rodata to some header
(I'd say a new one, linux/sections.h, which could then include
asm/sections.h for the declarations of __start_rodata,
__end_rodata). And then I'd move this block to a small helper and do

s = sanitize_slashes(s);
if (!s)
	return -ENOMEM;

or something. sanitize_slashes would be

char *t;

if (!strchr(s, '/))
	return s;
if (is_kernel_rodata(s)) {
	t = kstrdup(s, GFP_KERNEL);
	if (!t)
		return NULL;
} else {
	t = (char*)s;
}
strreplace(t, '/', '!');
return t;

But maybe that's knowing too much about how
kvasprintf_const/kstrdup_const work (for example, it would be bad if
they ever learned another unmodifiable section). In any case, would be
better as one or two follow-up patches.

Rasmus

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