On Wed, Sep 23, 2020 at 06:46:25PM +0100, Alan Maguire wrote:
> +
> +static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
> + u64 flags, const struct btf **btf,
> + s32 *btf_id)
> +{
> + u8 btf_kind = BTF_KIND_TYPEDEF;
> + char type_name[KSYM_NAME_LEN];
> + const struct btf_type *t;
> + const char *btf_type;
> + int ret;
> +
> + if (unlikely(flags & ~(BTF_F_ALL)))
> + return -EINVAL;
> +
> + if (btf_ptr_size != sizeof(struct btf_ptr))
> + return -EINVAL;
> +
> + *btf = bpf_get_btf_vmlinux();
> +
> + if (IS_ERR_OR_NULL(*btf))
> + return PTR_ERR(*btf);
> +
> + if (ptr->type != NULL) {
> + ret = copy_from_kernel_nofault(type_name, ptr->type,
> + sizeof(type_name));
nofault copy from bpf program global data... hmm...
I guess that works, but...
> + if (ret)
> + return ret;
> +
> + btf_type = type_name;
> +
> + if (strncmp(btf_type, "struct ", strlen("struct ")) == 0) {
> + btf_kind = BTF_KIND_STRUCT;
> + btf_type += strlen("struct ");
> + } else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
> + btf_kind = BTF_KIND_UNION;
> + btf_type += strlen("union ");
> + } else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
> + btf_kind = BTF_KIND_ENUM;
> + btf_type += strlen("enum ");
> + }
> +
> + if (strlen(btf_type) == 0)
> + return -EINVAL;
> +
> + /* Assume type specified is a typedef as there's not much
> + * benefit in specifying int types other than wasting time
> + * on BTF lookups; we optimize for the most useful path.
> + *
> + * Fall back to BTF_KIND_INT if this fails.
> + */
> + *btf_id = btf_find_by_name_kind(*btf, btf_type, btf_kind);
> + if (*btf_id < 0)
> + *btf_id = btf_find_by_name_kind(*btf, btf_type,
> + BTF_KIND_INT);
with all that fragility...
> + } else if (ptr->type_id > 0)
> + *btf_id = ptr->type_id;
since __builtin_btf_type_id() landed in llvm in February and it works
may be support type_id only?
Manually specifying type name as a string is error prone.
Plus that copy_from_kernel... which is doing copy from bpf prog.
I slept on it, but still feels unclean.
May be do type_id only for now and if we really really need string types
we can add it later after initial patches land?
