Re: unstable bpf helpers proposal. Was: [PATCH bpf-next v2 1/2] bpf: add bpf_ct_lookup_{tcp,udp}() helpers

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Matt Cover <werekraken@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Matthew Cover <matthew.cover@stackpath.com>,
	netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	kernel-team@fb.com
Subject: Re: unstable bpf helpers proposal. Was: [PATCH bpf-next v2 1/2] bpf: add bpf_ct_lookup_{tcp,udp}() helpers
Date: Tue, 7 Apr 2020 10:34:23 -0700	[thread overview]
Message-ID: <20200407173423.jh2ed3enaohfo5g2@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <CAGyo_hrYYYN6EXnbocauMo52pF52fAQwiJbDVZwH4NG3UC5anQ@mail.gmail.com>

On Mon, Apr 06, 2020 at 10:28:13PM -0700, Matt Cover wrote:
> >
> > I don't see why 1 and 2 are necessary.
> > What is the value of true export_symbol here?
> 
> Hmm... I was under the impression that these functions had to be
> exported to be eligible for BTF. Perhaps I'm misunderstanding this
> dwaves commit:
> 
>   3c5f2a224aa1 ("btf_encoder: Preserve and encode exported functions
> as BTF_KIND_FUNC")
> 
> Looking briefly I can see that the functions in symvers and BTF are
> not an exact match. Does "exported functions" in the above commit
> message not mean "exported symbols"?

Yeah. That pahole's commit log is confusing.
It meant to say that all of exported symbols will be in BTF
along with all other global functions.
$ bpftool btf dump file ./bld_x64/vmlinux|grep __icmp_send
[71784] FUNC '__icmp_send' type_id=71783
$ bpftool btf dump file ./bld_x64/vmlinux|grep bpf_prog_alloc_no_stats
[17945] FUNC 'bpf_prog_alloc_no_stats' type_id=17943
First one is exported. Second is a simple global.
There is no difference between them from BTF pov.

pahole can be improved too.
If it turns out that certain static functions has to be in BTF
we can easily make it so.

> >
> > crc don't add any additional value on top of BTF. BTF types has to match exactly.
> > It's like C compiler checking that you can call a function with correct proto.
> 
> I can see that for the verifier BTF is much superior to crc. The
> safety of the program is not improved by the crc. I was simply
> thinking the crc could be used in struct variant selection instead
> of kernel version. In some environments this could be useful since
> distros often backport patches while leaving version old (often
> meaning a second distro-specific version must also be considered).

The kernel version should not be used in any kind of logic. Lots of folks
backport bpf patches to older versions of the kernel. The kernel version is
meaningless form bpf pov. We even removed kernel_version check from kprobe
programs, because it was useless. vmlinux BTF is a complete description of the
running kernel. It's the one that the verifier is using to do it safety checks.