Re: [RFC PATCH bpf-next 06/16] bpf: add netlink and ipv6_route targets

Andrii Nakryiko
2020-04-10T23:13:52Z

On Wed, Apr 8, 2020 at 4:25 PM Yonghong Song <yhs@fb.com> wrote:
>
> This patch added netlink and ipv6_route targets, using
> the same seq_ops (except show()) for /proc/net/{netlink,ipv6_route}.
>
> Since module is not supported for now, ipv6_route is
> supported only if the IPV6 is built-in, i.e., not compiled
> as a module. The restriction can be lifted once module
> is properly supported for bpfdump.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 1 +
> kernel/bpf/dump.c | 13 ++++++++++
> net/ipv6/ip6_fib.c | 41 +++++++++++++++++++++++++++++-
> net/ipv6/route.c | 22 ++++++++++++++++
> net/netlink/af_netlink.c | 54 +++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 129 insertions(+), 2 deletions(-)
>

[...]

>
> +#if IS_BUILTIN(CONFIG_IPV6)
> +static int ipv6_route_prog_seq_show(struct bpf_prog *prog, struct seq_file *seq,
> + u64 seq_num, void *v)
> +{
> + struct ipv6_route_iter *iter = seq->private;
> + struct {
> + struct fib6_info *rt;
> + struct seq_file *seq;
> + u64 seq_num;
> + } ctx = {

So this anonymous struct definition has to match bpfdump__ipv6_route
function prototype, if I understand correctly. So this means that BTF
will have a very useful struct, that can be used directly in BPF
program, but it won't have a canonical name. This is very sad... Would
it be possible to instead use a struct as a prototype for these
dumpers? Here's why it matters. Instead of currently requiring BPF
users to declare their dumpers as (just copy-pasted):

int BPF_PROG(some_name, struct fib6_info *rt, struct seq_file *seq,
u64 seq_num) {
...
}

if bpfdump__ipv6_route was actually a struct definition:

struct bpfdump__ipv6_route {
struct fib6_info *rt;
struct seq_file *seq;
u64 seq_num;
};

Then with vmlinux.h, such program would be very nicely declared and used as:

int some_name(struct bpfdump__ipv6_route *ctx) {
/* here use ctx->rt, ctx->seq, ctx->seqnum */
}

This is would would be nice to have for raw_tp and tp_btf as well.

Of course we can also code-generate such types from func_protos in
bpftool, and that's a plan B for this, IMO. But seem like in this case
you already have two keep two separate entities in sync: func proto
and struct for context, so I thought I'd bring it up.

> + .rt = v,
> + .seq = seq,
> + .seq_num = seq_num,
> + };
> + int ret;
> +
> + ret = bpf_dump_run_prog(prog, &ctx);
> + iter->w.leaf = NULL;
> + return ret == 0 ? 0 : -EINVAL;
> +}
> +