| Message ID | 1520400451-11475-3-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 6, 2018 at 9:27 PM, Tobin C. Harding <me@tobin.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). > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > --- > drivers/staging/lustre/lustre/llite/xattr.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c > index 532384c91447..6f4099cd4afa 100644 > --- a/drivers/staging/lustre/lustre/llite/xattr.c > +++ b/drivers/staging/lustre/lustre/llite/xattr.c > @@ -36,6 +36,7 @@ > #include <linux/mm.h> > #include <linux/xattr.h> > #include <linux/selinux.h> > +#include <linux/vla.h> > > #define DEBUG_SUBSYSTEM S_LLITE > > @@ -87,12 +88,13 @@ ll_xattr_set_common(const struct xattr_handler *handler, > const char *name, const void *value, size_t size, > int flags) > { > - char fullname[strlen(handler->prefix) + strlen(name) + 1]; > struct ll_sb_info *sbi = ll_i2sbi(inode); > struct ptlrpc_request *req = NULL; > const char *pv = value; > __u64 valid; > int rc; > + int size = strlen(handler->prefix) + strlen(name) + 1; > + VLA_SAFE(char, fullname, size, VLA_DEFAULT_MAX); > > if (flags == XATTR_REPLACE) { > ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_REMOVEXATTR, 1); In the lustre case, I think it's better to just remove the stack allocation entirely. (See separate patch...) -Kees
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@tobin.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? I'm still trying to get my head around how we get rid of VLA when the stack is faster? Is this a speed vs safety trade off that must be tackled on a case by case basis? thanks, Tobin.
On Wed, Mar 07 2018, "Tobin C. Harding" <tobin@apporbit.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@tobin.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
On Wed, Mar 07, 2018 at 10:45:55PM +0100, Rasmus Villemoes wrote: > On Wed, Mar 07 2018, "Tobin C. Harding" <tobin@apporbit.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@tobin.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. Oh cool, good metric. thanks. > 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 So, I can go ahead and patch any place that uses *printf() with a VLA to use kasprintf() and iff performance is mentioned look at speeding up kasprintf(). I like it. thanks, Tobin.
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c index 532384c91447..6f4099cd4afa 100644 --- a/drivers/staging/lustre/lustre/llite/xattr.c +++ b/drivers/staging/lustre/lustre/llite/xattr.c @@ -36,6 +36,7 @@ #include <linux/mm.h> #include <linux/xattr.h> #include <linux/selinux.h> +#include <linux/vla.h> #define DEBUG_SUBSYSTEM S_LLITE @@ -87,12 +88,13 @@ ll_xattr_set_common(const struct xattr_handler *handler, const char *name, const void *value, size_t size, int flags) { - char fullname[strlen(handler->prefix) + strlen(name) + 1]; struct ll_sb_info *sbi = ll_i2sbi(inode); struct ptlrpc_request *req = NULL; const char *pv = value; __u64 valid; int rc; + int size = strlen(handler->prefix) + strlen(name) + 1; + VLA_SAFE(char, fullname, size, VLA_DEFAULT_MAX); if (flags == XATTR_REPLACE) { ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_REMOVEXATTR, 1);
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. Signed-off-by: Tobin C. Harding <me@tobin.cc> --- drivers/staging/lustre/lustre/llite/xattr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)