On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@xxxxxx> wrote:
>
> Add BTF_KIND_TAG support for parsing and dedup.
> Also added sanitization for BTF_KIND_TAG. If BTF_KIND_TAG is not
> supported in the kernel, sanitize it to INTs.
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> tools/lib/bpf/btf.c | 61 +++++++++++++++++++++++++++++++++
> tools/lib/bpf/btf.h | 13 +++++++
> tools/lib/bpf/btf_dump.c | 3 ++
> tools/lib/bpf/libbpf.c | 31 +++++++++++++++--
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 2 ++
> 6 files changed, 108 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 7cb6ebf1be37..ed02b17aad17 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -304,6 +304,8 @@ static int btf_type_size(const struct btf_type *t)
> return base_size + sizeof(struct btf_var);
> case BTF_KIND_DATASEC:
> return base_size + vlen * sizeof(struct btf_var_secinfo);
> + case BTF_KIND_TAG:
> + return base_size + sizeof(struct btf_tag);
> default:
> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
> return -EINVAL;
> @@ -376,6 +378,9 @@ static int btf_bswap_type_rest(struct btf_type *t)
> v->size = bswap_32(v->size);
> }
> return 0;
> + case BTF_KIND_TAG:
> + btf_tag(t)->comp_id = bswap_32(btf_tag(t)->comp_id);
> + return 0;
> default:
> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
> return -EINVAL;
> @@ -586,6 +591,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
> case BTF_KIND_CONST:
> case BTF_KIND_RESTRICT:
> case BTF_KIND_VAR:
> + case BTF_KIND_TAG:
> type_id = t->type;
> break;
> case BTF_KIND_ARRAY:
> @@ -2440,6 +2446,41 @@ int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __
> return 0;
> }
>
> +int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id)
Curious about the terminology here. The string recorded in bpf_tag, is
that a "name" of the tag, or rather a "value" of the tag? We should
reflect that in argument names for btf__add_tag.
I'll also nitpick on order of arguments. ref_type_id is always
specified, and it points to the entire type (struct/union/func), while
comp_id might, optionally, point inside that type. So I think the
order should be ref_type_id followed by comp_id.
Please also add a comment describing inputs (especially the -1 comp_id
case) and outputs, like all that other btf__add_xxx() APIs.
> +{
> + bool for_ref_type = false;
> + struct btf_type *t;
> + int sz, name_off;
> +
> + if (!name || !name[0] || comp_id < -1)
> + return libbpf_err(-EINVAL);
> +
> + if (validate_type_id(ref_type_id))
> + return libbpf_err(-EINVAL);
> +
> + if (btf_ensure_modifiable(btf))
> + return libbpf_err(-ENOMEM);
> +
> + sz = sizeof(struct btf_type) + sizeof(struct btf_tag);
> + t = btf_add_type_mem(btf, sz);
> + if (!t)
> + return libbpf_err(-ENOMEM);
> +
> + name_off = btf__add_str(btf, name);
> + if (name_off < 0)
> + return name_off;
> +
> + t->name_off = name_off;
> + t->type = ref_type_id;
> +
> + if (comp_id == -1)
> + for_ref_type = true;
> + t->info = btf_type_info(BTF_KIND_TAG, 0, for_ref_type);
> + ((struct btf_tag *)(t + 1))->comp_id = for_ref_type ? 0 : comp_id;
As I mentioned in the previous patch, it feels cleaner to just have
this -1 special value and not utilize kflag at all. It will match
libbpf API as you defined it naturally.
> +
> + return btf_commit_type(btf, sz);
> +}
> +
[...]
> case BTF_KIND_ARRAY: {
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..a78cf8331d49 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -141,6 +141,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
> LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
> __u32 offset, __u32 byte_sz);
>
> +/* tag contruction API */
typo: construction, but I'd put it after btf__add_restrict with no comment
> +LIBBPF_API int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id);
> +
> struct btf_dedup_opts {
> unsigned int dedup_table_size;
> bool dont_resolve_fwds;
> @@ -328,6 +331,11 @@ static inline bool btf_is_float(const struct btf_type *t)
> return btf_kind(t) == BTF_KIND_FLOAT;
> }
>
> +static inline bool btf_is_tag(const struct btf_type *t)
> +{
> + return btf_kind(t) == BTF_KIND_TAG;
> +}
> +
> static inline __u8 btf_int_encoding(const struct btf_type *t)
> {
> return BTF_INT_ENCODING(*(__u32 *)(t + 1));
> @@ -396,6 +404,11 @@ btf_var_secinfos(const struct btf_type *t)
> return (struct btf_var_secinfo *)(t + 1);
> }
>
please add `struct btf_tag;` forward reference for those users who are
compiling with old UAPI headers.
> +static inline struct btf_tag *btf_tag(const struct btf_type *t)
> +{
> + return (struct btf_tag *)(t + 1);
> +}
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
[...]
> LIBBPF_0.5.0 {
> global:
> + btf__add_tag;
> bpf_map__initial_value;
> bpf_map__pin_path;
> bpf_map_lookup_and_delete_elem_flags;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 533b0211f40a..7deb86d9af51 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -69,6 +69,8 @@
> #define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size)
> #define BTF_TYPE_FLOAT_ENC(name, sz) \
> BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
> +#define BTF_TYPE_TAG_KIND_ENC(name, type) \
following other macro names, it should be BTF_TYPE_TAG_ENC, no?
> + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
>
> #ifndef likely
> #define likely(x) __builtin_expect(!!(x), 1)
> --
> 2.30.2
>
