On Tue, Jun 28, 2022 at 9:02 AM Daniel Müller <deso@xxxxxxxxxx> wrote:
>
> This patch adds support for the proposed type match relation to
> relo_core where it is shared between userspace and kernel. A bit more
> plumbing is necessary and will arrive with subsequent changes to
> actually use it -- this patch only introduces the main matching
> algorithm.
>
> The matching relation is defined as follows (copy from source):
> - modifiers and typedefs are stripped (and, hence, effectively ignored)
> - generally speaking types need to be of same kind (struct vs. struct, union
> vs. union, etc.)
> - exceptions are struct/union behind a pointer which could also match a
> forward declaration of a struct or union, respectively, and enum vs.
> enum64 (see below)
> Then, depending on type:
> - integers:
> - match if size and signedness match
> - arrays & pointers:
> - target types are recursively matched
> - structs & unions:
> - local members need to exist in target with the same name
> - for each member we recursively check match unless it is already behind a
> pointer, in which case we only check matching names and compatible kind
> - enums:
> - local variants have to have a match in target by symbolic name (but not
> numeric value)
> - size has to match (but enum may match enum64 and vice versa)
> - function pointers:
> - number and position of arguments in local type has to match target
> - for each argument and the return value we recursively check match
>
> Signed-off-by: Daniel Müller <deso@xxxxxxxxxx>
> ---
> tools/lib/bpf/relo_core.c | 268 ++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/relo_core.h | 2 +
> 2 files changed, 270 insertions(+)
>
[...]
> +static int bpf_core_composites_match(const struct btf *local_btf, const struct btf_type *local_t,
> + const struct btf *targ_btf, const struct btf_type *targ_t,
> + bool behind_ptr, int level)
> +{
> + const struct btf_member *local_m = btf_members(local_t);
> + __u16 local_vlen = btf_vlen(local_t);
> + __u16 targ_vlen = btf_vlen(targ_t);
> + int i, j, err;
> +
> + if (local_vlen > targ_vlen)
> + return 0;
> +
> + /* check that all local members have a match in the target */
> + for (i = 0; i < local_vlen; i++, local_m++) {
> + const struct btf_member *targ_m = btf_members(targ_t);
> + bool matched = false;
> +
> + for (j = 0; j < targ_vlen; j++, targ_m++) {
> + if (!bpf_core_names_match(local_btf, local_m->name_off, targ_btf,
> + targ_m->name_off))
> + continue;
> +
> + err = __bpf_core_types_match(local_btf, local_m->type, targ_btf,
> + targ_m->type, behind_ptr, level - 1);
> + if (err > 0) {
this seems a bit too permissive, if we get an error, we should error
out instead of ignoring this. I left this for a follow up, though
> + matched = true;
> + break;
> + }
> + }
> +
> + if (!matched)
> + return 0;
> + }
> + return 1;
> +}
> +
> +/* Check that two types "match".
> + *
> + * The matching relation is defined as follows:
> + * - modifiers and typedefs are stripped (and, hence, effectively ignored)
> + * - generally speaking types need to be of same kind (struct vs. struct, union
> + * vs. union, etc.)
> + * - exceptions are struct/union behind a pointer which could also match a
> + * forward declaration of a struct or union, respectively, and enum vs.
> + * enum64 (see below)
> + * Then, depending on type:
> + * - integers:
> + * - match if size and signedness match
> + * - arrays & pointers:
> + * - target types are recursively matched
> + * - structs & unions:
> + * - local members need to exist in target with the same name
> + * - for each member we recursively check match unless it is already behind a
> + * pointer, in which case we only check matching names and compatible kind
> + * - enums:
> + * - local variants have to have a match in target by symbolic name (but not
> + * numeric value)
> + * - size has to match (but enum may match enum64 and vice versa)
> + * - function pointers:
> + * - number and position of arguments in local type has to match target
> + * - for each argument and the return value we recursively check match
> + */
> +int __bpf_core_types_match(const struct btf *local_btf, __u32 local_id, const struct btf *targ_btf,
> + __u32 targ_id, bool behind_ptr, int level)
> +{
> + const struct btf_type *local_t, *targ_t;
> + int depth = 32; /* max recursion depth */
> + __u16 local_k;
> +
> + if (level <= 0)
> + return -EINVAL;
> +
> + local_t = btf_type_by_id(local_btf, local_id);
> + targ_t = btf_type_by_id(targ_btf, targ_id);
> +
> +recur:
> + depth--;
> + if (depth < 0)
> + return -EINVAL;
> +
> + local_t = skip_mods_and_typedefs(local_btf, local_id, &local_id);
> + targ_t = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id);
> + if (!local_t || !targ_t)
> + return -EINVAL;
> +
> + if (!bpf_core_names_match(local_btf, local_t->name_off, targ_btf, targ_t->name_off))
> + return 0;
so the location of this check bothers me
Think about the case when we have on one side
typedef struct { /* something */ } abc;
and this on the other side:
typedef struct { /* something */ } def;
As this is written right now, we'll just ignore "abc" and "def" names,
which seems wrong. I haven't touched this part, but let's think what
to do about that and have a follow up patch.
> +
> + local_k = btf_kind(local_t);
> +
> + switch (local_k) {
[...]
> +
> + return bpf_core_enums_match(local_btf, local_t, targ_btf, targ_t);
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION: {
> + __u16 targ_k = btf_kind(targ_t);
you use targ_k almost in every case, so it would be cleaner to just
calculated right next to local_k, IMO (so that's what I did when
applying)
> +
> + if (behind_ptr) {
> + bool targ_f = BTF_INFO_KFLAG(targ_t->info);
[...]
> +
> + local_sgn = btf_int_encoding(local_t) & BTF_INT_SIGNED;
> + targ_sgn = btf_int_encoding(targ_t) & BTF_INT_SIGNED;
> +
> + return btf_int_bits(local_t) == btf_int_bits(targ_t) && local_sgn == targ_sgn;
nit: should probably be local_t->size == targ_t->size instead of
btf_int_bits() (which is kind of deprecated)
> + }
> + case BTF_KIND_PTR:
> + if (local_k != btf_kind(targ_t))
> + return 0;
> +
> + behind_ptr = true;
> +
> + local_id = local_t->type;
> + targ_id = targ_t->type;
> + goto recur;
[...]
