Messages in this thread | | | Date | Thu, 24 Sep 2020 17:42:29 -0700 | From | Alexei Starovoitov <> | Subject | Re: [PATCH v6 bpf-next 3/6] bpf: add bpf_snprintf_btf helper |
| |
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?
| |