On Mon, 7 Sep 2020 at 00:05, Martin KaFai Lau <kafai@xxxxxx> wrote: > > On Fri, Sep 04, 2020 at 12:23:55PM +0100, Lorenz Bauer 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. > I believe the second way, ".check_btf_id", can be removed. It currently ensures > it can accept socket types that has "struct sock_common" in it. > > Since then, btf_struct_ids_match() has been introduced which I think can be > used here. A ".btf_id" pointing to the "struct sock_common" alone is enough. Yes, I think that is a nice simplification! > > If the above is doable, is this change still needed? I find .btf_id is really unintuitive. By looking at struct bpf_func_proto is not at all clear how it's used. Having to use the correct number of BTF_UNUSED to match the right argument also seems really bug prone to me, and makes it unlikely that the BTF_ID_LIST will be reused across function prototypes. So I think this is still a good refactoring. That said, with your suggestion it would be possible to use a u32 (or *u32 to ease initialization) instead of struct btf_id_set*. > > > > > 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. > > -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
