Re: [PATCH bpf-next v2 10/20] net: bpf: add netlink and ipv6_route bpf_iter targets

From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next v2 10/20] net: bpf: add netlink and ipv6_route bpf_iter targets
Date: Wed, 6 May 2020 10:32:02 -0700	[thread overview]
Message-ID: <d1589865-b891-458b-3d24-acc4b4c4f504@fb.com> (raw)
In-Reply-To: <CAEf4BzbtR71iWPdNmjy0kvfQC4xQr+MFe6Vh2k6Kzu0cfsVVzg@mail.gmail.com>

On 5/5/20 10:21 PM, Andrii Nakryiko wrote:
> On Sun, May 3, 2020 at 11:29 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> This patch added netlink and ipv6_route targets, using
>> the same seq_ops (except show() and minor changes for stop())
>> for /proc/net/{netlink,ipv6_route}.
>>
>> The net namespace for these targets are the current net
>> namespace at file open stage, similar to
>> /proc/net/{netlink,ipv6_route} reference counting
>> the net namespace at seq_file open stage.
>>
>> 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 bpf_iter.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   fs/proc/proc_net.c       | 19 +++++++++
>>   include/linux/proc_fs.h  |  3 ++
>>   net/ipv6/ip6_fib.c       | 65 +++++++++++++++++++++++++++++-
>>   net/ipv6/route.c         | 27 +++++++++++++
>>   net/netlink/af_netlink.c | 87 +++++++++++++++++++++++++++++++++++++++-
>>   5 files changed, 197 insertions(+), 4 deletions(-)
>>
> 
> [...]
> 
>>   int __init ip6_route_init(void)
>>   {
>>          int ret;
>> @@ -6455,6 +6474,14 @@ int __init ip6_route_init(void)
>>          if (ret)
>>                  goto out_register_late_subsys;
>>
>> +#if IS_BUILTIN(CONFIG_IPV6)
>> +#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
>> +       ret = bpf_iter_register();
>> +       if (ret)
>> +               goto out_register_late_subsys;
> 
> Seems like bpf_iter infra is missing unregistering API.
> ip6_route_init(), if fails, undoes all the registrations, so probably
> should also unregister ipv6_route target as well?

Yes, it is. But not in this function. In this function, 
bpf_iter_register() is the last one possibly causing error,
so there is no need to unregister here.

But there is another cleanup funciton called outside of this
function, I need to do proper unregister there.

Thanks for catching this.

> 
>> +#endif
>> +#endif
>> +
>>          for_each_possible_cpu(cpu) {
>>                  struct uncached_list *ul = per_cpu_ptr(&rt6_uncached_list, cpu);
>>
> 
> [...]
> 
>> +static void netlink_seq_stop(struct seq_file *seq, void *v)
>> +{
>> +       struct bpf_iter_meta meta;
>> +       struct bpf_prog *prog;
>> +
>> +       if (!v) {
>> +               meta.seq = seq;
>> +               prog = bpf_iter_get_info(&meta, true);
>> +               if (prog)
>> +                       netlink_prog_seq_show(prog, &meta, v);
> 
> nit: netlink_prog_seq_show() can return failure (from BPF program),
> but you are not returning it. Given seq_file's stop is not supposed to
> fail, you can explicitly cast result to (void)? I think it's done in

Yes, we can do this. An explicit casting expressed the intention.

> few other places in BPF code, when return result is explicitly
> ignored.
> 
> 
>> +       }
>> +
>> +       netlink_native_seq_stop(seq, v);
>> +}
>> +#else
> 
> [...]
> 