On Fri, Jun 26, 2020 at 02:36:37PM -0700, Yonghong Song wrote:
SNIP
> > - }
> > -
> > - t = btf_type_by_id(btf_vmlinux, t->type);
> > - if (!btf_type_is_ptr(t))
> > - return -EFAULT;
> > - t = btf_type_by_id(btf_vmlinux, t->type);
> > - if (!btf_type_is_func_proto(t))
> > - return -EFAULT;
> > -
> > - args = (const struct btf_param *)(t + 1);
> > - if (arg >= btf_type_vlen(t)) {
> > - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n",
> > - fnname, arg);
> > + if (WARN_ON_ONCE(!fn->btf_id))
>
> The original code does not have this warning. It directly did
> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.
not sure why I put it in there, it's probably enough guarded
by arg_type having ARG_PTR_TO_BTF_ID, will remove
>
> > return -EINVAL;
> > - }
> > - t = btf_type_by_id(btf_vmlinux, args[arg].type);
> > - if (!btf_type_is_ptr(t) || !t->type) {
> > - /* anything but the pointer to struct is a helper config bug */
> > - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n");
> > - return -EFAULT;
> > - }
> > - btf_id = t->type;
> > - t = btf_type_by_id(btf_vmlinux, t->type);
> > - /* skip modifiers */
> > - while (btf_type_is_modifier(t)) {
> > - btf_id = t->type;
> > - t = btf_type_by_id(btf_vmlinux, t->type);
> > - }
> > - if (!btf_type_is_struct(t)) {
> > - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n");
> > - return -EFAULT;
> > - }
> > - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4,
> > - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off));
> > - return btf_id;
> > -}
> > + id = fn->btf_id[arg];
>
> The corresponding BTF_ID definition here is:
> BTF_ID_LIST(bpf_skb_output_btf_ids)
> BTF_ID(struct, sk_buff)
>
> The bpf helper writer needs to ensure proper declarations
> of BTF_IDs like the above matching helpers definition.
> Support we have arg1 and arg3 as BTF_ID. then the list
> definition may be
>
> BTF_ID_LIST(bpf_skb_output_btf_ids)
> BTF_ID(struct, sk_buff)
> BTF_ID(struct, __unused)
> BTF_ID(struct, task_struct)
>
> This probably okay, I guess.
right, AFAIK we don't have such case yet, but would be good
to be ready and have something like
BTF_ID(struct, __unused)
maybe adding new type for that will be better:
BTF_ID(none, unused)
jirka
