On Fri, Mar 26, 2021 at 2:53 PM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Mar 23, 2021 at 7:23 PM Florent Revest [off-list ref] wrote:

quoted

Two helpers (trace_printk and seq_printf) have very similar
implementations of format string parsing and a third one is coming
(snprintf). To avoid code duplication and make the code easier to
maintain, this moves the operations associated with format string
parsing (validation and argument sanitization) into one generic
function.

Unfortunately, the implementation of the two existing helpers already
drifted quite a bit and unifying them entailed a lot of changes:

"Unfortunately" as in a lot of extra work for you? I think overall
though it was very fortunate that you ended up doing it, all
implementations are more feature-complete and saner now, no? Thanks a
lot for your hard work!

quoted

- bpf_trace_printk always expected fmt[fmt_size] to be the terminating
  NULL character, this is no longer true, the first 0 is terminating.

You mean if you had bpf_trace_printk("bla bla\0some more bla\0", 24)
it would emit that zero character? If yes, I don't think it was a sane
behavior anyways.

quoted

- bpf_trace_printk now supports %% (which produces the percentage char).
- bpf_trace_printk now skips width formating fields.
- bpf_trace_printk now supports the X modifier (capital hexadecimal).
- bpf_trace_printk now supports %pK, %px, %pB, %pi4, %pI4, %pi6 and %pI6
- argument casting on 32 bit has been simplified into one macro and
  using an enum instead of obscure int increments.

- bpf_seq_printf now uses bpf_trace_copy_string instead of
  strncpy_from_kernel_nofault and handles the %pks %pus specifiers.
- bpf_seq_printf now prints longs correctly on 32 bit architectures.

- both were changed to use a global per-cpu tmp buffer instead of one
  stack buffer for trace_printk and 6 small buffers for seq_printf.
- to avoid per-cpu buffer usage conflict, these helpers disable
  preemption while the per-cpu buffer is in use.
- both helpers now support the %ps and %pS specifiers to print symbols.

Signed-off-by: Florent Revest <redacted>
---

This is great, you already saved some lines of code! I suspect I'll
have some complaints about mods (it feels like this preample should
provide extra information about which arguments have to be read from
kernel/user memory, but I'll see next patches first.

Disregard the last part (at least for now). I had a mental model that
it should be possible to parse a format string once and then remember
"instructions" (i.e., arg1 is long, arg2 is string, and so on). But
that's too complicated, so I think re-parsing the format string is
much simpler.

See my comments below (I deliberately didn't trim most of the code for
easier jumping around), but it's great overall, thanks!

quoted

 kernel/trace/bpf_trace.c | 529 ++++++++++++++++++---------------------
 1 file changed, 244 insertions(+), 285 deletions(-)

[...]

quoted

+int bpf_printf_preamble(char *fmt, u32 fmt_size, const u64 *raw_args,
+                       u64 *final_args, enum bpf_printf_mod_type *mod,
+                       u32 num_args)
+{
+       struct bpf_printf_buf *bufs = this_cpu_ptr(&bpf_printf_buf);
+       int err, i, fmt_cnt = 0, copy_size, used;
+       char *unsafe_ptr = NULL, *tmp_buf = NULL;
+       bool prepare_args = final_args && mod;

probably better to enforce that both or none are specified, otherwise
return error

it's actually three of them: raw_args, mod, and num_args, right? All
three are either NULL or non-NULL.

quoted

+       enum bpf_printf_mod_type current_mod;
+       size_t tmp_buf_len;
+       u64 current_arg;
+       char fmt_ptype;
+
+       for (i = 0; i < fmt_size && fmt[i] != '\0'; i++) {

[...]