Re: [PATCH bpf-next v1 2/7] bpf: Add MEM_RELEASE as a bpf_type_flag

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@fb.com> wrote:
>
> From: Joanne Koong <joannelkoong@gmail.com>
>
> 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@gmail.com>
> ---
>  include/linux/bpf.h          |  4 +++-
>  include/linux/bpf_verifier.h |  3 +--
>  kernel/bpf/btf.c             |  3 ++-
>  kernel/bpf/ringbuf.c         |  4 ++--
>  kernel/bpf/verifier.c        | 42 ++++++++++++++++++------------------
>  net/core/filter.c            |  2 +-
>  6 files changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6f2558da9d4a..cb9f42866cde 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -344,7 +344,9 @@ enum bpf_type_flag {
>
>         MEM_UNINIT              = BIT(5 + BPF_BASE_TYPE_BITS),
>
> -       __BPF_TYPE_LAST_FLAG    = MEM_UNINIT,
> +       MEM_RELEASE             = BIT(6 + BPF_BASE_TYPE_BITS),

"MEM_" part seems a bit too specific, it's not necessarily (just)
about memory, it's more generally about "releasing resources" in
general, right? ARG_RELEASE or OBJ_RELEASE maybe?

> +
> +       __BPF_TYPE_LAST_FLAG    = MEM_RELEASE,
>  };
>

[...]

> -/* Determine whether the function releases some resources allocated by another
> - * function call. The first reference type argument will be assumed to be
> - * released by release_reference().
> +/* Determine whether the type releases some resources allocated by a
> + * previous function call.
>   */
> -static bool is_release_function(enum bpf_func_id func_id)
> +static bool type_is_release_mem(u32 type)
>  {
> -       return func_id == BPF_FUNC_sk_release ||
> -              func_id == BPF_FUNC_ringbuf_submit ||
> -              func_id == BPF_FUNC_ringbuf_discard;
> +       return type & MEM_RELEASE;
>  }
>

same skepticism regarding the need for this helper function, just
makes grepping code slightly harder

>  static bool may_be_acquire_function(enum bpf_func_id func_id)

[...]