| Message ID | 20220620004233.3805-8-kent.overstreet@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Printbufs - new data structure for building strings | expand |
On 20/06/2022 02.42, Kent Overstreet wrote: > This makes printbufs optionally heap allocated: a printbuf initialized > with the PRINTBUF initializer will automatically heap allocate and > resize as needed. > > Allocations are done with GFP_KERNEL: code should use e.g. > memalloc_nofs_save()/restore() as needed. Since we do not currently have > memalloc_nowait_save()/restore(), in contexts where it is not safe to > block we provide the helpers > > printbuf_atomic_inc() > printbuf_atomic_dec() > > When the atomic count is nonzero, memory allocations will be done with > GFP_NOWAIT. > > On memory allocation failure, output will be truncated. Code that wishes > to check for memory allocation failure (in contexts where we should > return -ENOMEM) should check if printbuf->allocation_failure is set. > Since printbufs are expected to be typically used for log messages and > on a best effort basis, we don't return errors directly. > > Other helpers provided by this patch: > > - printbuf_make_room(buf, extra) > Reallocates if necessary to make room for @extra bytes (not including > terminating null). > > - printbuf_str(buf) > Returns a null terminated string equivalent to the contents of @buf. > If @buf was never allocated (or allocation failed), returns a > constant empty string. > > - printbuf_exit(buf) > Releases memory allocated by a printbuf. > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> > --- > include/linux/printbuf.h | 120 +++++++++++++++++++++++++++++++++------ > lib/Makefile | 2 +- > lib/printbuf.c | 71 +++++++++++++++++++++++ > 3 files changed, 175 insertions(+), 18 deletions(-) > create mode 100644 lib/printbuf.c > > diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h > index 8186c447ca..382863afa7 100644 > --- a/include/linux/printbuf.h > +++ b/include/linux/printbuf.h > @@ -4,19 +4,69 @@ > #ifndef _LINUX_PRINTBUF_H > #define _LINUX_PRINTBUF_H > > -#include <linux/kernel.h> > -#include <linux/string.h> > - > /* > - * Printbufs: String buffer for outputting (printing) to, for vsnprintf > + * Printbufs: Simple strings for printing to, with optional heap allocation > + * > + * This code has provisions for use in userspace, to aid in making other code > + * portable between kernelspace and userspace. > + * > + * Basic example: > + * struct printbuf buf = PRINTBUF; > + * > + * prt_printf(&buf, "foo="); > + * foo_to_text(&buf, foo); > + * printk("%s", buf.buf); So, if prt_printf() and foo_to_text() failed to make room, buf.buf would still be NULL, right? Sure, vsnprintf() handles that gracefully, but this is probably not what you intended. > struct printbuf { > char *buf; > unsigned size; > unsigned pos; > + /* > + * If nonzero, allocations will be done with GFP_ATOMIC: > + */ > + u8 atomic; > + bool allocation_failure:1; > + bool heap_allocated:1; > }; > > +int printbuf_make_room(struct printbuf *, unsigned); > +const char *printbuf_str(const struct printbuf *); > +void printbuf_exit(struct printbuf *); > + > +/* Initializer for a heap allocated printbuf: */ > +#define PRINTBUF ((struct printbuf) { .heap_allocated = true }) > + > +/* Initializer a printbuf that points to an external buffer: */ > +#define PRINTBUF_EXTERN(_buf, _size) \ > +((struct printbuf) { \ > + .buf = _buf, \ > + .size = _size, \ > +}) > + > /* > * Returns size remaining of output buffer: > */ > @@ -49,26 +99,36 @@ static inline bool printbuf_overflowed(struct printbuf *out) > > static inline void printbuf_nul_terminate(struct printbuf *out) > { > + printbuf_make_room(out, 1); > + Shouldn't this be printbuf_make_room(out, 0)? Probably this should be split up, so that the functions that also do printbuf_make_room() which ensures room for a nul-terminator could then call __printbuf_nul_terminate(), which would just contain the below: > if (out->pos < out->size) > out->buf[out->pos] = 0; > else if (out->size) > out->buf[out->size - 1] = 0; > } > -static inline void __prt_char(struct printbuf *out, char c) > +/* Doesn't call printbuf_make_room(), doesn't nul terminate: */ > +static inline void __prt_char_reserved(struct printbuf *out, char c) > { > if (printbuf_remaining(out)) > out->buf[out->pos] = c; > out->pos++; > } > > +/* Doesn't nul terminate: */ > +static inline void __prt_char(struct printbuf *out, char c) > +{ > + printbuf_make_room(out, 1); > + __prt_char_reserved(out, c); > +} > + > static inline void prt_char(struct printbuf *out, char c) > { > __prt_char(out, c); > printbuf_nul_terminate(out); > } > > -static inline void __prt_chars(struct printbuf *out, char c, unsigned n) > +static inline void __prt_chars_reserved(struct printbuf *out, char c, unsigned n) > { > unsigned i, can_print = min(n, printbuf_remaining(out)); > > @@ -79,13 +139,18 @@ static inline void __prt_chars(struct printbuf *out, char c, unsigned n) > > static inline void prt_chars(struct printbuf *out, char c, unsigned n) > { > - __prt_chars(out, c, n); > + printbuf_make_room(out, n); > + __prt_chars_reserved(out, c, n); > printbuf_nul_terminate(out); > } > > static inline void prt_bytes(struct printbuf *out, const void *b, unsigned n) > { > - unsigned i, can_print = min(n, printbuf_remaining(out)); > + unsigned i, can_print; > + > + printbuf_make_room(out, n); > + > + can_print = min(n, printbuf_remaining(out)); > > for (i = 0; i < can_print; i++) > out->buf[out->pos++] = ((char *) b)[i]; > @@ -101,22 +166,43 @@ static inline void prt_str(struct printbuf *out, const char *str) > > static inline void prt_hex_byte(struct printbuf *out, u8 byte) > { > - __prt_char(out, hex_asc_hi(byte)); > - __prt_char(out, hex_asc_lo(byte)); > + printbuf_make_room(out, 2); > + __prt_char_reserved(out, hex_asc_hi(byte)); > + __prt_char_reserved(out, hex_asc_lo(byte)); > printbuf_nul_terminate(out); > } > > static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte) > { > - __prt_char(out, hex_asc_upper_hi(byte)); > - __prt_char(out, hex_asc_upper_lo(byte)); > + printbuf_make_room(out, 2); > + __prt_char_reserved(out, hex_asc_upper_hi(byte)); > + __prt_char_reserved(out, hex_asc_upper_lo(byte)); > printbuf_nul_terminate(out); > } > > -#define PRINTBUF_EXTERN(_buf, _size) \ > -((struct printbuf) { \ > - .buf = _buf, \ > - .size = _size, \ > -}) > +/** > + * printbuf_reset - re-use a printbuf without freeing and re-initializing it: > + */ > +static inline void printbuf_reset(struct printbuf *buf) > +{ > + buf->pos = 0; > + buf->allocation_failure = 0; > +} > + > +/** > + * printbuf_atomic_inc - mark as entering an atomic section > + */ > +static inline void printbuf_atomic_inc(struct printbuf *buf) > +{ > + buf->atomic++; > +} > + > +/** > + * printbuf_atomic_inc - mark as leaving an atomic section > + */ > +static inline void printbuf_atomic_dec(struct printbuf *buf) > +{ > + buf->atomic--; > +} So, if I have a printbuf in scope, and I do irq_disable() or spin_lock() or whatnot, I'm supposed to also call printbuf_atomic_inc(), at least if the printbuf is used within the locked region. Honest question: An u8 used for this purpose cannot overflow? > #endif /* _LINUX_PRINTBUF_H */ > diff --git a/lib/Makefile b/lib/Makefile > index 6b9ffc1bd1..b4609a4258 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -34,7 +34,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \ > is_single_threaded.o plist.o decompress.o kobject_uevent.o \ > earlycpio.o seq_buf.o siphash.o dec_and_lock.o \ > nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \ > - buildid.o > + buildid.o printbuf.o > > lib-$(CONFIG_PRINTK) += dump_stack.o > lib-$(CONFIG_SMP) += cpumask.o > diff --git a/lib/printbuf.c b/lib/printbuf.c > new file mode 100644 > index 0000000000..8c70128e31 > --- /dev/null > +++ b/lib/printbuf.c > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: LGPL-2.1+ > +/* Copyright (C) 2022 Kent Overstreet */ > + > +#ifdef __KERNEL__ > +#include <linux/export.h> > +#include <linux/kernel.h> > +#else > +#define EXPORT_SYMBOL(x) > +#endif > + > +#include <linux/err.h> > +#include <linux/slab.h> > +#include <linux/printbuf.h> > + > +int printbuf_make_room(struct printbuf *out, unsigned extra) > +{ > + unsigned new_size; > + char *buf; > + > + if (!out->heap_allocated) > + return 0; I think that ->allocation_failure should be sticky and make us return an early error here; if we're under memory pressure we don't want each and every prt_char() of whatever we're trying to print to end up trying to do an allocation. > + /* Reserved space for terminating nul: */ > + extra += 1; > + > + if (out->pos + extra < out->size) > + return 0; Are you sure you don't want to be careful about the possibility of out->pos+extra overflowing? And since extra has been ++'ed, shouldn't the comparison be <= ? If pos is 0, size is 2, and I want to add one char (so on entry extra is 1), this should not require a reallocation? > + new_size = roundup_pow_of_two(out->size + extra); Are you sure you don't want to be careful about the possibility of out->size+extra overflowing, or hitting that with roundup_pow_of_two() doing that? > + buf = krealloc(out->buf, new_size, !out->atomic ? GFP_KERNEL : GFP_NOWAIT); > + > + if (!buf) { > + out->allocation_failure = true; > + return -ENOMEM; > + } > + > + out->buf = buf; > + out->size = new_size; > + return 0; > +} > +EXPORT_SYMBOL(printbuf_make_room); > + > +/** > + * printbuf_str - returns printbuf's buf as a C string, guaranteed to be null Pet peeve: Please use the spelling "nul" consistently. > + * terminated > + */ > +const char *printbuf_str(const struct printbuf *buf) > +{ > + /* > + * If we've written to a printbuf then it's guaranteed to be a null > + * terminated string - but if we haven't, then we might not have > + * allocated a buffer at all: > + */ > + return buf->pos > + ? buf->buf > + : ""; > +} > +EXPORT_SYMBOL(printbuf_str); I think the documentation lacks some mention of lifetimes and caller obligations or lack thereof. Especially since the return value could become dangling not just if the printbuf is destroyed (printbuf_exit), but also any other use of the printbuf which could cause a realloc. Rasmus
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h index 8186c447ca..382863afa7 100644 --- a/include/linux/printbuf.h +++ b/include/linux/printbuf.h @@ -4,19 +4,69 @@ #ifndef _LINUX_PRINTBUF_H #define _LINUX_PRINTBUF_H -#include <linux/kernel.h> -#include <linux/string.h> - /* - * Printbufs: String buffer for outputting (printing) to, for vsnprintf + * Printbufs: Simple strings for printing to, with optional heap allocation + * + * This code has provisions for use in userspace, to aid in making other code + * portable between kernelspace and userspace. + * + * Basic example: + * struct printbuf buf = PRINTBUF; + * + * prt_printf(&buf, "foo="); + * foo_to_text(&buf, foo); + * printk("%s", buf.buf); + * printbuf_exit(&buf); + * + * Or + * struct printbuf buf = PRINTBUF_EXTERN(char_buf, char_buf_size) + * + * We can now write pretty printers instead of writing code that dumps + * everything to the kernel log buffer, and then those pretty-printers can be + * used by other code that outputs to kernel log, sysfs, debugfs, etc. + * + * Memory allocation: Outputing to a printbuf may allocate memory. This + * allocation is done with GFP_KERNEL, by default: use the newer + * memalloc_*_(save|restore) functions as needed. + * + * Since no equivalent yet exists for GFP_ATOMIC/GFP_NOWAIT, memory allocations + * will be done with GFP_NOWAIT if printbuf->atomic is nonzero. + * + * Memory allocation failures: We don't return errors directly, because on + * memory allocation failure we usually don't want to bail out and unwind - we + * want to print what we've got, on a best-effort basis. But code that does want + * to return -ENOMEM may check printbuf.allocation_failure. */ +#include <linux/kernel.h> +#include <linux/string.h> + struct printbuf { char *buf; unsigned size; unsigned pos; + /* + * If nonzero, allocations will be done with GFP_ATOMIC: + */ + u8 atomic; + bool allocation_failure:1; + bool heap_allocated:1; }; +int printbuf_make_room(struct printbuf *, unsigned); +const char *printbuf_str(const struct printbuf *); +void printbuf_exit(struct printbuf *); + +/* Initializer for a heap allocated printbuf: */ +#define PRINTBUF ((struct printbuf) { .heap_allocated = true }) + +/* Initializer a printbuf that points to an external buffer: */ +#define PRINTBUF_EXTERN(_buf, _size) \ +((struct printbuf) { \ + .buf = _buf, \ + .size = _size, \ +}) + /* * Returns size remaining of output buffer: */ @@ -49,26 +99,36 @@ static inline bool printbuf_overflowed(struct printbuf *out) static inline void printbuf_nul_terminate(struct printbuf *out) { + printbuf_make_room(out, 1); + if (out->pos < out->size) out->buf[out->pos] = 0; else if (out->size) out->buf[out->size - 1] = 0; } -static inline void __prt_char(struct printbuf *out, char c) +/* Doesn't call printbuf_make_room(), doesn't nul terminate: */ +static inline void __prt_char_reserved(struct printbuf *out, char c) { if (printbuf_remaining(out)) out->buf[out->pos] = c; out->pos++; } +/* Doesn't nul terminate: */ +static inline void __prt_char(struct printbuf *out, char c) +{ + printbuf_make_room(out, 1); + __prt_char_reserved(out, c); +} + static inline void prt_char(struct printbuf *out, char c) { __prt_char(out, c); printbuf_nul_terminate(out); } -static inline void __prt_chars(struct printbuf *out, char c, unsigned n) +static inline void __prt_chars_reserved(struct printbuf *out, char c, unsigned n) { unsigned i, can_print = min(n, printbuf_remaining(out)); @@ -79,13 +139,18 @@ static inline void __prt_chars(struct printbuf *out, char c, unsigned n) static inline void prt_chars(struct printbuf *out, char c, unsigned n) { - __prt_chars(out, c, n); + printbuf_make_room(out, n); + __prt_chars_reserved(out, c, n); printbuf_nul_terminate(out); } static inline void prt_bytes(struct printbuf *out, const void *b, unsigned n) { - unsigned i, can_print = min(n, printbuf_remaining(out)); + unsigned i, can_print; + + printbuf_make_room(out, n); + + can_print = min(n, printbuf_remaining(out)); for (i = 0; i < can_print; i++) out->buf[out->pos++] = ((char *) b)[i]; @@ -101,22 +166,43 @@ static inline void prt_str(struct printbuf *out, const char *str) static inline void prt_hex_byte(struct printbuf *out, u8 byte) { - __prt_char(out, hex_asc_hi(byte)); - __prt_char(out, hex_asc_lo(byte)); + printbuf_make_room(out, 2); + __prt_char_reserved(out, hex_asc_hi(byte)); + __prt_char_reserved(out, hex_asc_lo(byte)); printbuf_nul_terminate(out); } static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte) { - __prt_char(out, hex_asc_upper_hi(byte)); - __prt_char(out, hex_asc_upper_lo(byte)); + printbuf_make_room(out, 2); + __prt_char_reserved(out, hex_asc_upper_hi(byte)); + __prt_char_reserved(out, hex_asc_upper_lo(byte)); printbuf_nul_terminate(out); } -#define PRINTBUF_EXTERN(_buf, _size) \ -((struct printbuf) { \ - .buf = _buf, \ - .size = _size, \ -}) +/** + * printbuf_reset - re-use a printbuf without freeing and re-initializing it: + */ +static inline void printbuf_reset(struct printbuf *buf) +{ + buf->pos = 0; + buf->allocation_failure = 0; +} + +/** + * printbuf_atomic_inc - mark as entering an atomic section + */ +static inline void printbuf_atomic_inc(struct printbuf *buf) +{ + buf->atomic++; +} + +/** + * printbuf_atomic_inc - mark as leaving an atomic section + */ +static inline void printbuf_atomic_dec(struct printbuf *buf) +{ + buf->atomic--; +} #endif /* _LINUX_PRINTBUF_H */ diff --git a/lib/Makefile b/lib/Makefile index 6b9ffc1bd1..b4609a4258 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -34,7 +34,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \ is_single_threaded.o plist.o decompress.o kobject_uevent.o \ earlycpio.o seq_buf.o siphash.o dec_and_lock.o \ nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \ - buildid.o + buildid.o printbuf.o lib-$(CONFIG_PRINTK) += dump_stack.o lib-$(CONFIG_SMP) += cpumask.o diff --git a/lib/printbuf.c b/lib/printbuf.c new file mode 100644 index 0000000000..8c70128e31 --- /dev/null +++ b/lib/printbuf.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: LGPL-2.1+ +/* Copyright (C) 2022 Kent Overstreet */ + +#ifdef __KERNEL__ +#include <linux/export.h> +#include <linux/kernel.h> +#else +#define EXPORT_SYMBOL(x) +#endif + +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/printbuf.h> + +int printbuf_make_room(struct printbuf *out, unsigned extra) +{ + unsigned new_size; + char *buf; + + if (!out->heap_allocated) + return 0; + + /* Reserved space for terminating nul: */ + extra += 1; + + if (out->pos + extra < out->size) + return 0; + + new_size = roundup_pow_of_two(out->size + extra); + buf = krealloc(out->buf, new_size, !out->atomic ? GFP_KERNEL : GFP_NOWAIT); + + if (!buf) { + out->allocation_failure = true; + return -ENOMEM; + } + + out->buf = buf; + out->size = new_size; + return 0; +} +EXPORT_SYMBOL(printbuf_make_room); + +/** + * printbuf_str - returns printbuf's buf as a C string, guaranteed to be null + * terminated + */ +const char *printbuf_str(const struct printbuf *buf) +{ + /* + * If we've written to a printbuf then it's guaranteed to be a null + * terminated string - but if we haven't, then we might not have + * allocated a buffer at all: + */ + return buf->pos + ? buf->buf + : ""; +} +EXPORT_SYMBOL(printbuf_str); + +/** + * printbuf_exit - exit a printbuf, freeing memory it owns and poisoning it + * against accidental use. + */ +void printbuf_exit(struct printbuf *buf) +{ + if (buf->heap_allocated) { + kfree(buf->buf); + buf->buf = ERR_PTR(-EINTR); /* poison value */ + } +} +EXPORT_SYMBOL(printbuf_exit);
This makes printbufs optionally heap allocated: a printbuf initialized with the PRINTBUF initializer will automatically heap allocate and resize as needed. Allocations are done with GFP_KERNEL: code should use e.g. memalloc_nofs_save()/restore() as needed. Since we do not currently have memalloc_nowait_save()/restore(), in contexts where it is not safe to block we provide the helpers printbuf_atomic_inc() printbuf_atomic_dec() When the atomic count is nonzero, memory allocations will be done with GFP_NOWAIT. On memory allocation failure, output will be truncated. Code that wishes to check for memory allocation failure (in contexts where we should return -ENOMEM) should check if printbuf->allocation_failure is set. Since printbufs are expected to be typically used for log messages and on a best effort basis, we don't return errors directly. Other helpers provided by this patch: - printbuf_make_room(buf, extra) Reallocates if necessary to make room for @extra bytes (not including terminating null). - printbuf_str(buf) Returns a null terminated string equivalent to the contents of @buf. If @buf was never allocated (or allocation failed), returns a constant empty string. - printbuf_exit(buf) Releases memory allocated by a printbuf. Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> --- include/linux/printbuf.h | 120 +++++++++++++++++++++++++++++++++------ lib/Makefile | 2 +- lib/printbuf.c | 71 +++++++++++++++++++++++ 3 files changed, 175 insertions(+), 18 deletions(-) create mode 100644 lib/printbuf.c