Re: [PATCH v6 bpf-next 3/6] bpf: add bpf_snprintf_btf helper

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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?