Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Apr 8, 2020 at 4:26 PM Yonghong Song <yhs@fb.com> wrote:
>
> Given a loaded dumper bpf program, which already
> knows which target it should bind to, there
> two ways to create a dumper:
>   - a file based dumper under hierarchy of
>     /sys/kernel/bpfdump/ which uses can
>     "cat" to print out the output.
>   - an anonymous dumper which user application
>     can "read" the dumping output.
>
> For file based dumper, BPF_OBJ_PIN syscall interface
> is used. For anonymous dumper, BPF_PROG_ATTACH
> syscall interface is used.
>
> To facilitate target seq_ops->show() to get the
> bpf program easily, dumper creation increased
> the target-provided seq_file private data size
> so bpf program pointer is also stored in seq_file
> private data.
>
> Further, a seq_num which represents how many
> bpf_dump_get_prog() has been called is also
> available to the target seq_ops->show().
> Such information can be used to e.g., print
> banner before printing out actual data.

So I looked up seq_operations struct and did a very cursory read of
fs/seq_file.c and seq_file documentation, so I might be completely off
here.

start() is called before iteration begins, stop() is called after
iteration ends. Would it be a bit better and user-friendly interface
to have to extra calls to BPF program, say with NULL input element,
but with extra enum/flag that specifies that this is a START or END of
iteration, in addition to seq_num?

Also, right now it's impossible to write stateful dumpers that do any
kind of stats calculation, because it's impossible to determine when
iteration restarted (it starts from the very beginning, not from the
last element). It's impossible to just rememebr last processed
seq_num, because BPF program might be called for a new "session" in
parallel with the old one.

So it seems like few things would be useful:

1. end flag for post-aggregation and/or footer printing (seq_num == 0
is providing similar means for start flag).
2. Some sort of "session id", so that bpfdumper can maintain
per-session intermediate state. Plus with this it would be possible to
detect restarts (if there is some state for the same session and
seq_num == 0, this is restart).

It seems like it might be a bit more flexible to, instead of providing
seq_file * pointer directly, actually provide a bpfdumper_context
struct, which would have seq_file * as one of fields, other being
session_id and start/stop flags.

A bit unstructured thoughts, but what do you think?

>
> Note the seq_num does not represent the num
> of unique kernel objects the bpf program has
> seen. But it should be a good approximate.
>
> A target feature BPF_DUMP_SEQ_NET_PRIVATE
> is implemented specifically useful for
> net based dumpers. It sets net namespace
> as the current process net namespace.
> This avoids changing existing net seq_ops
> in order to retrieve net namespace from
> the seq_file pointer.
>
> For open dumper files, anonymous or not, the
> fdinfo will show the target and prog_id associated
> with that file descriptor. For dumper file itself,
> a kernel interface will be provided to retrieve the
> prog_id in one of the later patches.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/bpf.h            |   5 +
>  include/uapi/linux/bpf.h       |   6 +-
>  kernel/bpf/dump.c              | 338 ++++++++++++++++++++++++++++++++-
>  kernel/bpf/syscall.c           |  11 +-
>  tools/include/uapi/linux/bpf.h |   6 +-
>  5 files changed, 362 insertions(+), 4 deletions(-)
>

[...]