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@xxxxxxxxx>
> ---
> 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
