[PATCH v5whatever, now with typechecking] vsprintf: %pf(%p)
From: Kent Overstreet
Date: Mon Jun 27 2022 - 23:24:48 EST
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@xxxxxxxxx>
Reviewed-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Petr Mladek <pmladek@xxxxxxxx>
---
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