Messages in this thread Patch in this message | | | From | Kent Overstreet <> | Subject | [PATCH v5whatever, now with typechecking] vsprintf: %pf(%p) | Date | Mon, 27 Jun 2022 23:24:39 -0400 |
| |
Linus, here's an updated version of the patch implementing %pf(%p) that implements your typechecking macro and cookie idea we discussed. I haven't done any additional checking like the whitelist I was talking about - this feels sufficient to me for now, but I'm happy to revisit it if it comes up.
It's been tested lightly.
I would _really_ like to be able to pass integer arguments as well - if we want to go ahead with converting existing %p extensions to this it would be really helpful, and I was wondering if you had any thoughts on that.
I'm not sure if we can depend on integers and pointers being passed the same way at the ABI level - I got some screaming when I floated the idea, but after looking at actual ABI docs I couldn't find any archs that were _that_ crazy. The right way to do this would seem to be with libffi - it supports all Linux architectures and is designed exactly for this, and compiles down to practically nothing (under a kilobyte) with the features we don't need turned off. If pulling that into the kernel is something you'd be ok with, I can look more at it.
It'll be a bit before I'm ready to mail out the next version of the full printbuf patch series. I decided to go ahead with converting all our existing pretty-printers in lib/vsprintf.c to printbuf style:
- it makes the code _vastly_ easier to read and understand by separating out format string parsing from the pretty-printers, and giving actual named arguments to the pretty-printers
- if we _do_ decide to go ahead with the treewide conversion (and I think we should, since it'll give us typechecking that we don't have currently), we'll want to do them all at once.
-- >8 - This implements a new %p format string extensions for passing a pretty printer and its arguments to printk, which will then be inserted into the formatted output.
A pretty-printer is a function that takes as its first argument a pointer to a struct printbuf, and then zero or more additional pointer arguments - these being the objects to format and print.
The arguments to the pretty-printer function are denoted in the format string by %p, i.e %pf() foo0_to_text(struct printbuf *out) %pf(%p) foo1_to_text(struct printbuf *out, struct foo *) %pf(%p,%p) foo2_to_text(struct printbuf *out, struct foo *)
We'd also like to eventually support non pointer arguments - in particular, integers - but this will probably require libffi.
Typechecking is accomplished with the CALL_PP macro, which verifies that the arguments passed to sprintf match the types of the pp-function arguments, and passes a struct with a cookie to sprintf so that sprintf can verify that the CALL_PP() macro was used.
Full example:
static void foo_to_text(struct printbuf *out, struct foo *foo) { prt_printf(out, "bar=%u baz=%u", foo->bar, foo->baz); }
printf("%pf(%p)", CALL_PP(foo_to_text, foo));
The goal is to replace most of our %p format extensions with this interface, and to move pretty-printers out of the core vsprintf.c code - this will get us better organization and better discoverability (you'll be able to cscope to pretty printer calls!), as well as eliminate a lot of dispatch code in vsprintf.c.
Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Petr Mladek <pmladek@suse.com> --- Documentation/core-api/printk-formats.rst | 22 +++++ include/linux/printbuf.h | 26 ++++++ lib/test_printf.c | 27 ++++++ lib/vsprintf.c | 100 +++++++++++++++++++++- 4 files changed, 172 insertions(+), 3 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 5e89497ba3..4f4a35b3aa 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -625,6 +625,28 @@ Examples:: %p4cc Y10 little-endian (0x20303159) %p4cc NV12 big-endian (0xb231564e) +Calling a pretty printer function +--------------------------------- + +:: + + %pf(%p) pretty printer function taking one argument + %pf(%p,%p) pretty printer function taking two arguments + +For calling generic pretty printers. A pretty printer is a function that takes +as its first argument a pointer to a printbuf, and then zero or more additional +pointer arguments. For example: + + void foo_to_text(struct printbuf *out, struct foo *foo) + { + pr_buf(out, "bar=%u baz=%u", foo->bar, foo->baz); + } + + printf("%pf(%p)", CALL_PP(foo_to_text, foo)); + +Note that a pretty-printer may not sleep if called from printk(). If called from +prt_printf() or sprintf() there are no such restrictions. + Thanks ====== diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h index 8186c447ca..189828d48d 100644 --- a/include/linux/printbuf.h +++ b/include/linux/printbuf.h @@ -119,4 +119,30 @@ static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte) .size = _size, \ }) +/* + * This is used for the %pf(%p) sprintf format extension, where we pass a pretty + * printer and arguments to the pretty-printer to sprintf + * + * Instead of passing a pretty-printer function to sprintf directly, we pass it + * a pointer to a struct call_pp, so that sprintf can check that the magic + * number is present, which in turn ensures that the CALL_PP() macro has been + * used in order to typecheck the arguments to the pretty printer function + * + * Example usage: + * sprintf("%pf(%p)", CALL_PP(prt_bdev, bdev)); + */ +struct call_pp { + unsigned long magic; + void *fn; +}; + +#define PP_TYPECHECK(fn, ...) \ + ({ while (0) fn((struct printbuf *) NULL, ##__VA_ARGS__); }) + +#define CALL_PP_MAGIC (unsigned long) 0xce0b92d22f6b6be4 + +#define CALL_PP(fn, ...) \ + (PP_TYPECHECK(fn, ##__VA_ARGS__), \ + &((struct call_pp) { CALL_PP_MAGIC, fn })), ##__VA_ARGS__ + #endif /* _LINUX_PRINTBUF_H */ diff --git a/lib/test_printf.c b/lib/test_printf.c index 07309c45f3..af0c1c2d2d 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -9,6 +9,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/printk.h> +#include <linux/printbuf.h> #include <linux/random.h> #include <linux/rtc.h> #include <linux/slab.h> @@ -783,6 +784,31 @@ test_pointer(void) fourcc_pointer(); } +static void printf_test_fn_0(struct printbuf *out) +{ + prt_str(out, "0"); +} + +static void printf_test_fn_1(struct printbuf *out, void *p) +{ + int *i = p; + + prt_printf(out, "%i", *i); +} + +static void __init +test_fn(void) +{ + int i = 1; + + test("0", "%pf()", CALL_PP(printf_test_fn_0)); + test("1", "%pf(%p)", CALL_PP(printf_test_fn_1, &i)); + /* + * Not tested, so we don't fail the build with -Werror: + */ + //test("1", "%(%p)", printf_test_fn, &i); +} + static void __init selftest(void) { alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL); @@ -794,6 +820,7 @@ static void __init selftest(void) test_number(); test_string(); test_pointer(); + test_fn(); kfree(alloced_buffer); } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7b24714674..eb57524c5d 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -436,7 +436,8 @@ enum format_type { FORMAT_TYPE_UINT, FORMAT_TYPE_INT, FORMAT_TYPE_SIZE_T, - FORMAT_TYPE_PTRDIFF + FORMAT_TYPE_PTRDIFF, + FORMAT_TYPE_FN, }; struct printf_spec { @@ -2520,8 +2521,14 @@ int format_decode(const char *fmt, struct printf_spec *spec) return ++fmt - start; case 'p': - spec->type = FORMAT_TYPE_PTR; - return ++fmt - start; + fmt++; + if (fmt[0] == 'f' && + fmt[1] == '(') { + fmt += 2; + spec->type = FORMAT_TYPE_FN; + } else + spec->type = FORMAT_TYPE_PTR; + return fmt - start; case '%': spec->type = FORMAT_TYPE_PERCENT_CHAR; @@ -2602,6 +2609,67 @@ set_precision(struct printf_spec *spec, int prec) } } +static void call_prt_fn(struct printbuf *out, struct call_pp *call_pp, void **fn_args, unsigned nr_args) +{ + typedef void (*printf_fn_0)(struct printbuf *); + typedef void (*printf_fn_1)(struct printbuf *, void *); + typedef void (*printf_fn_2)(struct printbuf *, void *, void *); + typedef void (*printf_fn_3)(struct printbuf *, void *, void *, void *); + typedef void (*printf_fn_4)(struct printbuf *, void *, void *, void *, void *); + typedef void (*printf_fn_5)(struct printbuf *, void *, void *, void *, void *, void *); + typedef void (*printf_fn_6)(struct printbuf *, void *, void *, void *, void *, void *, void *); + typedef void (*printf_fn_7)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *); + typedef void (*printf_fn_8)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *, void *); + void *fn; + unsigned i; + + if (check_pointer(out, call_pp)) + return; + + if (call_pp->magic != CALL_PP_MAGIC) { + error_string(out, "bad pretty-printer magic"); + return; + } + + fn = call_pp->fn; + if (check_pointer(out, fn)) + return; + + for (i = 0; i < nr_args; i++) + if (check_pointer(out, fn_args[i])) + return; + + switch (nr_args) { + case 0: + ((printf_fn_0)fn)(out); + break; + case 1: + ((printf_fn_1)fn)(out, fn_args[0]); + break; + case 2: + ((printf_fn_2)fn)(out, fn_args[0], fn_args[1]); + break; + case 3: + ((printf_fn_3)fn)(out, fn_args[0], fn_args[1], fn_args[2]); + break; + case 4: + ((printf_fn_4)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3]); + break; + case 5: + ((printf_fn_5)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4]); + break; + case 6: + ((printf_fn_6)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5]); + break; + case 7: + ((printf_fn_7)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6]); + break; + case 8: + ((printf_fn_8)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6], fn_args[7]); + break; + } +} + /** * prt_vprintf - Format a string, outputting to a printbuf * @out: The printbuf to output to @@ -2665,6 +2733,32 @@ void prt_vprintf(struct printbuf *out, const char *fmt, va_list args) fmt++; break; + case FORMAT_TYPE_FN: { + unsigned nr_args = 0; + void *fn_args[8]; + void *fn = va_arg(args, void *); + + while (*fmt != ')') { + if (nr_args) { + if (fmt[0] != ',') + goto out; + fmt++; + } + + if (fmt[0] != '%' || fmt[1] != 'p') + goto out; + fmt += 2; + + if (WARN_ON_ONCE(nr_args == ARRAY_SIZE(fn_args))) + goto out; + fn_args[nr_args++] = va_arg(args, void *); + } + + call_prt_fn(out, fn, fn_args, nr_args); + fmt++; /* past trailing ) */ + break; + } + case FORMAT_TYPE_PERCENT_CHAR: __prt_char(out, '%'); break; -- 2.36.1
| |