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

[Yonghong Song <yhs@fb.com>]

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