On Sun, Nov 20, 2022 at 11:36:51PM +0530, Kumar Kartikeya Dwivedi wrote:
[...]
> > Not your change, but this is an awkwardly phrased error message. IMO
> > "dynptr must be initialized" is more succinct. Feel free to ignore if
> > you'd like, I'm happy to submit a separate patch to change it as some
> > point.
> >
>
> Feel free to, since I think unrelated changes should not be mixed in this patch.
No problem, will do.
[...]
> > > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > > @@ -6119,11 +6216,17 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > > if (arg_type_is_release(arg_type)) {
> > > if (arg_type_is_dynptr(arg_type)) {
> > > struct bpf_func_state *state = func(env, reg);
> > > - int spi = get_spi(reg->off);
> > > + int spi;
> > >
> > > - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> > > - !state->stack[spi].spilled_ptr.id) {
> > > - verbose(env, "arg %d is an unacquired reference\n", regno);
> >
> > Can we add a comment here explaining why only PTR_TO_STACK dynptrs are
> > expected to be released? I know we have such comments elsewhere already,
> > but if we're going to have logic like this which is hard-coded against
> > assumptions of what types of dynptrs can be used in which contexts /
> > helpers, I think it is important to be verbose in calling that out as
> > it's not obvious from the code itself why this is the case.
> >
>
> Sure, but you mean a code comment, right?
Yep, a code comment.
[...]
