On Wed, 9 Sep 2020 at 06:57, Martin KaFai Lau <kafai@xxxxxx> wrote: > > 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. Sounds good to me, I'll give this a try. -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
