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@xxxxxx> 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. > > 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. 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.
