Re: [PATCH v3 bpf-next 02/11] bpf: Enable bpf_skc_to_* sock casting helper to networking prog type

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Sep 24, 2020 at 09:38:09AM +0100, Lorenz Bauer wrote:
> On Wed, 23 Sep 2020 at 18:06, Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 10:27:27AM +0100, Lorenz Bauer wrote:
> > > On Tue, 22 Sep 2020 at 19:26, Martin KaFai Lau <kafai@fb.com> wrote:
> > > >
> > > > On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote:
> > > > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote:
> > > > > >
> > > > > > There is a constant need to add more fields into the bpf_tcp_sock
> > > > > > for the bpf programs running at tc, sock_ops...etc.
> > > > > >
> > > > > > A current workaround could be to use bpf_probe_read_kernel().  However,
> > > > > > other than making another helper call for reading each field and missing
> > > > > > CO-RE, it is also not as intuitive to use as directly reading
> > > > > > "tp->lsndtime" for example.  While already having perfmon cap to do
> > > > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can
> > > > > > directly read from the tcp_sock.
> > > > > >
> > > > > > This patch tries to do that by using the existing casting-helpers
> > > > > > bpf_skc_to_*() whose func_proto returns a btf_id.  For example, the
> > > > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the
> > > > > > kernel "struct tcp_sock".
> > > > > >
> > > > > > These helpers are also added to is_ptr_cast_function().
> > > > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id.
> > > > > > That will keep the ref-tracking works properly.
> > > > > >
> > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog
> > > > > > types in filter.c. They are limited by perfmon cap.
> > > > > >
> > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON.  The helper accepting
> > > > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON])
> > > > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON).  The bpf_skc_to_*()
> > > > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that
> > > > > > they will accept pointer obtained from skb->sk.
> > > > > >
> > > > > > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON
> > > > > > at verification time.  All PTR_TO_*_OR_NULL reg has to do a NULL check
> > > > > > first before passing into the helper or else the bpf prog will be
> > > > > > rejected by the verifier.
> > > > > >
> > > > > > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier.  The _OR_NULL was
> > > > > >   needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL
> > > > > >   PTR_TO_BTF_ID is not a scalar NULL to the verifier.  "_OR_NULL" implicitly
> > > > > >   gives an expectation that the helper can take a scalar NULL which does
> > > > > >   not make sense in most (except one) helpers.  Passing scalar NULL
> > > > > >   should be rejected at the verification time.
> > > > >
> > > > > What is the benefit of requiring a !sk check from the user if all of
> > > > > the helpers know how to deal with a NULL pointer?
> > > > I don't see a reason why the verifier should not reject an incorrect
> > > > program at load time if it can.
> > > >
> > > > >
> > > > > >
> > > > > >   Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the
> > > > > >   helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but
> > > > > >   not scalar NULL.  It requires the func_proto to explicitly specify the
> > > > > >   arg_btf_id such that there is a very clear expectation that the helper
> > > > > >   can handle a NULL PTR_TO_BTF_ID. ]
> > > > >
> > > > > I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since
> > > > > nothing enforces that arg_btf_id is actually an ID for sock common.
> > > > > This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to
> > > > > understand, even though it's more permissive than it has to be. It
> > > > > communicates very clearly what values the argument can take.
> > > > _OR_NULL is incorrect which implies a scalar NULL as mentioned in
> > > > this commit message.  From verifier pov, _OR_NULL can take
> > > > a scalar NULL.
> > >
> > > Yes, I know. I'm saying that the distinction between scalar NULL and
> > > runtime NULL only makes sense after you understand how BTF pointers
> > > are implemented. It only clicked for me after I read the support code
> > > in the JIT that Yonghong pointed out. Should everybody that writes a
> > > helper need to read the JIT? In my opinion we shouldn't. I guess I
> > > don't even care about the verifier rejecting scalar NULL or not, I'd
> > > just like the types to have a name that conveys their NULLness.
> > It is not only about verifier and/or JIT, not sure why it is related to
> > JIT also.
> >
> > For some helpers, explicitly passing NULL may make sense.
> > e.g. bpf_sk_assign(ctx, NULL, 0) makes sense.
> >
> > For most helpers, the bpf prog is wrong for sure, for example
> > in sockmap, what does bpf_map_update_elem(sock_map, key, NULL, 0)
> > mean?  I would expect a delete from the sock_map if the verifier
> > accepted it.
> >
> > >
> > > >
> > > > >
> > > > > If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the
> > > > > btf_id in struct bpf_reg_types. This avoids the weird case where the
> > > > > btf_id doesn't actually point at sock_common, and it also makes my
> > > > I have considered the bpf_reg_types option.  I prefer all
> > > > arg info (arg_type and arg_btf_id) stay in the same one
> > > > place (i.e. func_proto) as much as possible for now
> > > > instead of introducing another place to specify/override it
> > > > which then depends on a particular arg_type that some arg_type may be
> > > > in func_proto while some may be in other places.
> > >
> > > In my opinion that ship sailed when we started aliasing arg_type to
> > > multiple reg_type, but OK.
> > >
> > > >
> > > > The arg_btf_id can be checked in check_btf_id_ok() if it would be a
> > > > big concern that it might slip through the review but I think the
> > > > chance is pretty low.
> > >
> > > Why increase the burden on human reviewers? Why add code to check an
> > > invariant that we could get rid of in the first place?
> > Lets take the scalar NULL example that requires to read multiple
> > pieces of codes in different places (verifier, JIT...etc.).
> > As you also mentioned, yes, it may be easy for a few people.
> > However, for most others, having some obvious things in the same place is
> > easier to review.
> >
> > I think we have to agree we disagree on this one implementation details
> > which I think it has been over-thought (and time also).
> >
> > If you insist that should go into bpf_reg_types (i.e. compatible->btf_id),
> > I can do that in v4 and then add another check in another place to
> > ensure "!compatible->btf_id" as in v2.
> 
> No, I don't insist. I was hoping I could convince you, but alas :)

Not very convinced but I don't feel very strongly on this ;)
so I have posted v4 with btf_id in struct bpf_reg_types to continue
the review of the set.  Thanks.