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

Yonghong Song
2020-04-10T23:53:07Z

On 4/10/20 4:13 PM, Andrii Nakryiko wrote:
> 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 */
> }

Thanks, I do not know this!
This definitely better and may make kernel code simpler.
Will experiment.

>
> 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;
>> +}
>> +