On Mon, Dec 16, 2019 at 10:14:09PM -0800, Yonghong Song wrote:
[ ... ]
> > +void bpf_struct_ops_init(struct btf *_btf_vmlinux)
> > +{
> > + const struct btf_member *member;
> > + struct bpf_struct_ops *st_ops;
> > + struct bpf_verifier_log log = {};
> > + const struct btf_type *t;
> > + const char *mname;
> > + s32 type_id;
> > + u32 i, j;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> > + st_ops = bpf_struct_ops[i];
> > +
> > + type_id = btf_find_by_name_kind(_btf_vmlinux, st_ops->name,
> > + BTF_KIND_STRUCT);
> > + if (type_id < 0) {
> > + pr_warn("Cannot find struct %s in btf_vmlinux\n",
> > + st_ops->name);
> > + continue;
> > + }
> > + t = btf_type_by_id(_btf_vmlinux, type_id);
> > + if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
> > + pr_warn("Cannot support #%u members in struct %s\n",
> > + btf_type_vlen(t), st_ops->name);
> > + continue;
> > + }
> > +
> > + for_each_member(j, t, member) {
> > + const struct btf_type *func_proto;
> > +
> > + mname = btf_name_by_offset(_btf_vmlinux,
> > + member->name_off);
> > + if (!*mname) {
> > + pr_warn("anon member in struct %s is not supported\n",
> > + st_ops->name);
> > + break;
> > + }
> > +
> > + if (btf_member_bitfield_size(t, member)) {
> > + pr_warn("bit field member %s in struct %s is not supported\n",
> > + mname, st_ops->name);
> > + break;
> > + }
> > +
> > + func_proto = btf_type_resolve_func_ptr(_btf_vmlinux,
> > + member->type,
> > + NULL);
> > + if (func_proto &&
> > + btf_distill_func_proto(&log, _btf_vmlinux,
> > + func_proto, mname,
> > + &st_ops->func_models[j])) {
> > + pr_warn("Error in parsing func ptr %s in struct %s\n",
> > + mname, st_ops->name);
> > + break;
> > + }
> > + }
> > +
> > + if (j == btf_type_vlen(t)) {
> > + if (st_ops->init(_btf_vmlinux)) {
>
> is it possible that st_ops->init might be a NULL pointer?
Not now. The check could be added if there would be
struct_ops that does not need init.
>
> > + pr_warn("Error in init bpf_struct_ops %s\n",
> > + st_ops->name);
> > + } else {
> > + st_ops->type_id = type_id;
> > + st_ops->type = t;
> > + }
> > + }
> > + }
> > +}
[ ... ]
> > @@ -6343,8 +6353,30 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
> > static int check_return_code(struct bpf_verifier_env *env)
> > {
> > struct tnum enforce_attach_type_range = tnum_unknown;
> > + const struct bpf_prog *prog = env->prog;
> > struct bpf_reg_state *reg;
> > struct tnum range = tnum_range(0, 1);
> > + int err;
> > +
> > + /* The struct_ops func-ptr's return type could be "void" */
> > + if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS &&
> > + !prog->aux->attach_func_proto->type)
> > + return 0;
> > +
> > + /* eBPF calling convetion is such that R0 is used
> > + * to return the value from eBPF program.
> > + * Make sure that it's readable at this time
> > + * of bpf_exit, which means that program wrote
> > + * something into it earlier
> > + */
> > + err = check_reg_arg(env, BPF_REG_0, SRC_OP);
> > + if (err)
> > + return err;
> > +
> > + if (is_pointer_value(env, BPF_REG_0)) {
> > + verbose(env, "R0 leaks addr as return value\n");
> > + return -EACCES;
> > + }
> >
> > switch (env->prog->type) {
> > case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> > @@ -8010,21 +8042,6 @@ static int do_check(struct bpf_verifier_env *env)
> > if (err)
> > return err;
> >
> > - /* eBPF calling convetion is such that R0 is used
> > - * to return the value from eBPF program.
> > - * Make sure that it's readable at this time
> > - * of bpf_exit, which means that program wrote
> > - * something into it earlier
> > - */
> > - err = check_reg_arg(env, BPF_REG_0, SRC_OP);
> > - if (err)
> > - return err;
> > -
> > - if (is_pointer_value(env, BPF_REG_0)) {
> > - verbose(env, "R0 leaks addr as return value\n");
> > - return -EACCES;
> > - }
> > -
> > err = check_return_code(env);
> > if (err)
> > return err;
> > @@ -8833,12 +8850,14 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> > convert_ctx_access = bpf_xdp_sock_convert_ctx_access;
> > break;
> > case PTR_TO_BTF_ID:
> > - if (type == BPF_WRITE) {
> > + if (type == BPF_READ) {
> > + insn->code = BPF_LDX | BPF_PROBE_MEM |
> > + BPF_SIZE((insn)->code);
> > + env->prog->aux->num_exentries++;
> > + } else if (env->prog->type != BPF_PROG_TYPE_STRUCT_OPS) {
> > verbose(env, "Writes through BTF pointers are not allowed\n");
> > return -EINVAL;
> > }
> > - insn->code = BPF_LDX | BPF_PROBE_MEM | BPF_SIZE((insn)->code);
> > - env->prog->aux->num_exentries++;
>
> Do we need to increase num_exentries for BPF_WRITE as well?
Not needed since it does not need to gen exentry
for this write access for BPF_PROG_TYPE_STRUCT_OPS.
The individual struct_ops (e.g. the bpf_tcp_ca_btf_struct_access()
in patch 7) ensures the write is fine, which is like the
current convert_ctx_access() in filter.c but with the BTF help.
I will add some comments on this.
Thanks for the review!
