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?