On Tue, Sep 08, 2020 at 09:47:04PM -0700, Andrii Nakryiko wrote: > On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote: > > > > Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal > > which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of > > IDs, one for each argument. This array is only accessed up to the highest > > numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than > > five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id > > is a function pointer that is called by the verifier if present. It gets the > > actual BTF ID of the register, and the argument number we're currently checking. > > It turns out that the only user check_arg_btf_id ignores the argument, and is > > simply used to check whether the BTF ID matches one of the socket types. > > > > Replace both of these mechanisms with explicit btf_id_sets for each argument > > in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one > > of several IDs, and the code that does the type checking becomes simpler. > > > > Add a small optimisation to btf_set_contains for the common case of a set with > > a single entry. > > > > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx> > > --- > > You are replacing a more generic and powerful capability with a more > restricted one because no one is yet using a generic one fully. It > might be ok and we'll never need a more generic way to check BTF IDs. > But it will be funny if we will be adding this back soon because a > static set of BTF IDs don't cut it for some cases :) > > I don't mind this change, but I wonder what others think about this. With btf_struct_ids_match(), the only check_btf_id() use case is gone. It is better to keep one way of doing thing. The check_btf_id can be added back if there is a need. I think this only existing check_btf_id() use case should be removed and consolidate to the bpf_func_proto.btf_id. Also, for the "struct btf_id_set *arg_btf_ids[5]" change, there is currently no use case that a helper can take two different btf_ids (i.e. two different kernel structs) at the verification time. The btf_id_set will always have one element then. May be we can cross that bridge when there is a solid use case.
