[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 07 Mar 2018 22:45:55 +0100
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: "Tobin C. Harding" <tobin@...orbit.com>
Cc: Kees Cook <keescook@...omium.org>, "Tobin C. Harding" <me@...in.cc>, Kernel Hardening <kernel-hardening@...ts.openwall.com>, Tycho Andersen <tycho@...ho.ws>, Oleg Drokin <oleg.drokin@...el.com>, Andreas Dilger <andreas.dilger@...el.com>, James Simmons <jsimmons@...radead.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [RFC 2/2] lustre: use VLA_SAFE
On Wed, Mar 07 2018, "Tobin C. Harding" <tobin@...orbit.com> wrote:
> On Tue, Mar 06, 2018 at 09:46:02PM -0800, Kees Cook wrote:
>> On Tue, Mar 6, 2018 at 9:27 PM, Tobin C. Harding <me@...in.cc> wrote:
>> > Currently lustre uses a VLA to store a string on the stack. We can use
>> > the newly define VLA_SAFE macro to make this declaration safer.
>> >
>> > Use VLA_SAFE to declare VLA.
>>
>> VLA_SAFE implements a max, which is nice, but I think we're just
>> digging ourselves into a bigger hole with this, since now all the
>> maxes must be validated (which isn't done here, what happens if
>> VLA_DEFAULT_MAX is smaller than the strlen() math? We'll overflow the
>> stack buffer in the later sprintf).
>
> ok, lets drop this.
>
> Memory on the stack is always going to be faster than memory from the
> slub allocator, right? Do you think using kasprintf() is going to be
> acceptable? Isn't it only going to be acceptable on non-time critical
> paths?
Probably anything that does string formatting to begin with (using any
*printf function) is not performance critical. That e.g. applies to the
lustre case. It's true that kasprintf() ends up doing the vsnprintf()
twice, but I haven't yet seen an example where that is really a
problem. Should it actually come up, I've thought about a few things one
might do to partly mitigate that:
(1) Use that the vast majority of kasprintf() results are short'ish
strings, so instead of letting the first vsnprintf() write nothing at
all, allocate a smallish (32/64) stack buffer and do the first
vsnprintf() to that - on "success", just kmemdup() the result instead of
doing the formatting again.
(2) Add a
char *kasprintf_buf(buf, len, gfp, fmt, ...)
public API that will use the user-provided (buf, len) as initial target,
and return buf on success, otherwise fall back to kmalloc'ing an
appropriate buffer and redoing the formatting. It's not totally unheard
of to decide whether to kfree() based on comparison with a caller-owned
pointer. Typically buf would be stack-allocated, and it's up to the
caller to decide what a reasonable size would be - and especially useful
in cases such as the lustre one where the string does not outlive the
sprintf() caller.
(1) is easily implemented in terms of this.
(3) As (2), but with the rule being that buf must be kmalloc'ed, and
kasprintf() will kfree() it and allocate a new buffer if
necessary. Probably too hard to use (and exact semantics on failure
would need to be decided; like realloc(), or is the passed-in too-small
buffer already kfree'd?)
Rasmus
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
