On Fri, Oct 12, 2018 at 11:54:42AM -0700, Yonghong Song wrote: > This patch added interface to load a program with the following > additional information: > . prog_btf_fd > . func_info and func_info_len > where func_info will provides function range and type_id > corresponding to each function. > > If verifier agrees with function range provided by the user, > the bpf_prog ksym for each function will use the func name > provided in the type_id, which is supposed to provide better > encoding as it is not limited by 16 bytes program name > limitation and this is better for bpf program which contains > multiple subprograms. > > The bpf_prog_info interface is also extended to > return btf_id and jited_func_types, so user spaces can > print out the function prototype for each jited function. > > Signed-off-by: Yonghong Song <y...@fb.com> ... > BUILD_BUG_ON(sizeof("bpf_prog_") + > sizeof(prog->tag) * 2 + > @@ -401,6 +403,13 @@ static void bpf_get_prog_name(const struct bpf_prog > *prog, char *sym) > > sym += snprintf(sym, KSYM_NAME_LEN, "bpf_prog_"); > sym = bin2hex(sym, prog->tag, sizeof(prog->tag)); > + > + if (prog->aux->btf) { > + func_name = btf_get_name_by_id(prog->aux->btf, > prog->aux->type_id); > + snprintf(sym, (size_t)(end - sym), "_%s", func_name); > + return;
Would it make sense to add a comment here that prog->aux->name is ignored when full btf name is available? (otherwise the same name will appear twice in ksym) > + } > + > if (prog->aux->name[0]) > snprintf(sym, (size_t)(end - sym), "_%s", prog->aux->name); ... > +static int check_btf_func(struct bpf_prog *prog, struct bpf_verifier_env > *env, > + union bpf_attr *attr) > +{ > + struct bpf_func_info *data; > + int i, nfuncs, ret = 0; > + > + if (!attr->func_info_len) > + return 0; > + > + nfuncs = attr->func_info_len / sizeof(struct bpf_func_info); > + if (env->subprog_cnt != nfuncs) { > + verbose(env, "number of funcs in func_info does not match > verifier\n"); 'does not match verifier' is hard to make sense of. How about 'number of funcs in func_info doesn't match number of subprogs' ? > + return -EINVAL; > + } > + > + data = kvmalloc(attr->func_info_len, GFP_KERNEL | __GFP_NOWARN); > + if (!data) { > + verbose(env, "no memory to allocate attr func_info\n"); I don't think we ever print such warnings for memory allocations. imo this can be removed, since enomem is enough. > + return -ENOMEM; > + } > + > + if (copy_from_user(data, u64_to_user_ptr(attr->func_info), > + attr->func_info_len)) { > + verbose(env, "memory copy error for attr func_info\n"); similar thing. kernel never warns about copy_from_user errors. > + ret = -EFAULT; > + goto cleanup; > + } > + > + for (i = 0; i < nfuncs; i++) { > + if (env->subprog_info[i].start != data[i].insn_offset) { > + verbose(env, "func_info subprog start (%d) does not > match verifier (%d)\n", > + env->subprog_info[i].start, > data[i].insn_offset); I think printing exact insn offset isn't going to be much help for regular user to debug it. If this happens, it's likely llvm issue. How about 'func_info BTF section doesn't match subprog layout in BPF program' ?