On Sun, Mar 5, 2023 at 3:46 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sat, Mar 04, 2023 at 03:27:46PM -0800, Andrii Nakryiko wrote:
> > particular type. This will automatically work better for kfuncs as
> > new/next/destroy trios will have the same `struct bpf_iter_<type> *`
> > and it won't be possible to accidentally pass wrong bpf_iter_<type> to
> > wrong new/next/destroy method.
>
> Exactly.
>
cool, I'll change this to `struct bpf_iter_<type>`
[...]
>
> > > > +static bool is_iter_next_insn(struct bpf_verifier_env *env, int insn_idx, int *reg_idx)
> > > > +{
> > > > + struct bpf_insn *insn = &env->prog->insnsi[insn_idx];
> > > > + const struct btf_param *args;
> > > > + const struct btf_type *t;
> > > > + const struct btf *btf;
> > > > + int nargs, i;
> > > > +
> > > > + if (!bpf_pseudo_kfunc_call(insn))
> > > > + return false;
> > > > + if (!is_iter_next_kfunc(insn->imm))
> > > > + return false;
> > > > +
> > > > + btf = find_kfunc_desc_btf(env, insn->off);
> > > > + if (IS_ERR(btf))
> > > > + return false;
> > > > +
> > > > + t = btf_type_by_id(btf, insn->imm); /* FUNC */
> > > > + t = btf_type_by_id(btf, t->type); /* FUNC_PROTO */
> > > > +
> > > > + args = btf_params(t);
> > > > + nargs = btf_vlen(t);
> > > > + for (i = 0; i < nargs; i++) {
> > > > + if (is_kfunc_arg_iter(btf, &args[i])) {
> > > > + *reg_idx = BPF_REG_1 + i;
> > > > + return true;
> > > > + }
> > > > + }
> > >
> > > This is some future-proofing ?
> > > The commit log says that all iterators has to in the form:
> > > bpf_iter_<kind>_next(struct bpf_iter* it)
> > > Should we check for one and only arg here instead?
> >
> > Yeah, a bit of generality. For a long time I had an assumption
> > hardcoded about first argument being struct bpf_iter *, but that felt
> > unclean, so I generalized that before submission.
> >
> > But I can't think why we wouldn't just dictate (and enforce) that
> > `struct bpf_iter *` is first. It makes sense, it's clean, and we lose
> > nothing. This is another thing that differs between dynptr and iter,
> > for dynptr such restriction wouldn't make sense.
> >
> > Where would be a good place to enforce this for iter kfuncs?
>
> I would probably just remove is_iter_next_insn() completely, hard code BPF_REG_1
> and add a big comment for now.
>
> In the follow up we can figure out how to:
> BUILD_BUG_ON(!__same_type(bpf_iter_num_next, some canonical proto for iter_next));
>
> Like we do:
> BUILD_BUG_ON(!__same_type(ops->map_lookup_elem,
> (void *(*)(struct bpf_map *map, void *key))NULL));
>
I ended up adding proper enforcement of iter kfunc prototypes and
generalizing everything with new KF_ITER_xxx flags. It's a pretty
major change that allows implementing new iterators even easier,
please see v2. It's now possible to define iterators in kernel modules
as well, thanks to not having to update core verifier logic for each
new set of iter functions.
> > >
> > > 'depth' part of bpf_reg_state will be checked for equality in regsafe(), right?
> >
> > no, it is explicitly skipped (and it's actually stacksafe(), not
> > regsafe()). I can add explicit comment that we *ignore* depth
>
> Ahh. That's stacksafe() indeed.
> Would be great to add a comment to:
> + if (old_reg->iter.type != cur_reg->iter.type ||
> + old_reg->iter.state != cur_reg->iter.state ||
> + !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap))
> + return false;
>
> that depth is explicitly not compared.
ok, added
>
> > I was considering adding a flag to states_equal() whether to check
> > depth or not (that would make iter_active_depths_differ unnecessary),
> > but it doesn't feel right. Any preferences one way or the other?
>
> probably overkill. just comment should be enough.
>
[...]
>
> > >
> > > > + }
> > > > + /* attempt to detect infinite loop to avoid unnecessary doomed work */
> > > > + if (states_maybe_looping(&sl->state, cur) &&
> > >
> > > Maybe cleaner is to remove above 'goto' and do '} else if (states_maybe_looping' here ?
> >
> > I can undo this, it felt cleaner with explicit "skip infinite loop
> > check" both for new code and for that async_entry_cnt check above. But
> > I can revert to if/else if/else if pattern, though I find it harder to
> > follow, given all this code (plus comments) is pretty long, so it's
> > easy to lose track when reading
>
> I'm fine whichever way. I just remembered that you typically try to avoid goto-s
> and seeing goto here that could have easily been 'else' raised my internal alarm
> that I could be missing something subtle in the code here.
Well, it depends, as usual. I certainly don't like something like
"goto process_bpf_exit" in do_check(), which jumps into the middle of
a completely unrelated if/else branch, so there's that. But these
"skip a bunch of checks" gotos are cleaner, IMO, if we are talking
about a bunch of complicated if/else if/else checks. Let's go with
explicit goto for now, we can always revert.
>
> >
> > >
> > > > + states_equal(env, &sl->state, cur) &&
> > > > + !iter_active_depths_differ(&sl->state, cur)) {
> > > > verbose_linfo(env, insn_idx, "; ");
> > > > verbose(env, "infinite loop detected at insn %d\n", insn_idx);
> > > > return -EINVAL;
