On Mon, Apr 4, 2022 at 12:34 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Sat, Apr 02, 2022 at 07:28:21AM IST, Joanne Koong wrote:
> > From: Joanne Koong <joannelkoong@xxxxxxxxx>
> >
> > Currently, we hardcode in the verifier which functions are release
> > functions. We have no way of differentiating which argument is the one
> > to be released (we assume it will always be the first argument).
> >
> > This patch adds MEM_RELEASE as a bpf_type_flag. This allows us to
> > determine which argument in the function needs to be released, and
> > removes having to hardcode a list of release functions into the
> > verifier.
> >
> > Please note that currently, we only support one release argument in a
> > helper function. In the future, if/when we need to support several
> > release arguments within the function, MEM_RELEASE is necessary
> > since there needs to be a way of differentiating which arguments are the
> > release ones.
> >
> > In the near future, MEM_RELEASE will be used by dynptr helper functions
> > such as bpf_free.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
[...]
> > @@ -6693,7 +6693,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > return err;
> > }
> >
> > - if (is_release_function(func_id)) {
> > + if (meta.ref_obj_id) {
>
> The meta.ref_obj_id field is set unconditionally whenever we see a
> reg->ref_obj_id, e.g. when we pass a refcounted argument to non-release
> function. Wouldn't making this conditional only on meta.ref_obj_id lead to
> release of that register now? Or did I miss some change above which prevents
> this case?
>
Yes, unfortunately you are right. This wouldn't work for the cases
where a refcounted arg is passed to a non-release function, since that
also sets the meta.ref_obj_id. Thanks for catching this!
> To make things clear, I'm talking of this sequence:
>
> p = acquire();
> helper_foo(p); // meta.ref_obj_id would be set, and p is released
> release(p); // error, as p.ref_obj_id has no reference state
>
> Besides, in my series this PTR_RELEASE / MEM_RELEASE tagging is only needed
> because the release function can take a NULL pointer, so we need to know the
> register of the argument to be released, and then make sure it is refcounted,
> otherwise it must be NULL (and whether NULL is permitted or not is checked
> earlier during argument checks). That doesn't seem to be true for bpf_free in
> your series, as it can only take ARG_PTR_TO_DYNPTR (but maybe it should also
> set PTR_MAYBE_NULL).
>
In the dynptr case, there will be several release-type functions (eg
bpf_free, bpf_ringbuf_discard, bpf_ringbuf_submit). The motivation
behind this patch was to have some way of signifying this instead of
having to specify in the verifier the particular functions. Please let
me know if this addresses your comment or if there's something in
between the lines in your reply that I'm missing
> > err = release_reference(env, meta.ref_obj_id);
> > if (err) {
> > verbose(env, "func %s#%d reference has not been acquired before\n",
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 9aafec3a09ed..a935ce7a63bc 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6621,7 +6621,7 @@ static const struct bpf_func_proto bpf_sk_release_proto = {
> > .func = bpf_sk_release,
> > .gpl_only = false,
> > .ret_type = RET_INTEGER,
> > - .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON | MEM_RELEASE,
> > };
> >
> > BPF_CALL_5(bpf_xdp_sk_lookup_udp, struct xdp_buff *, ctx,
> > --
> > 2.30.2
> >
>
> --
> Kartikeya
