On Thu, 14 Jul 2022 at 01:46, Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> The may_be_acquire_function check is a weaker version of
> is_acquire_function that only uses bpf_func_id to determine whether a
> func may be acquiring a reference. Most funcs which acquire a reference
> do so regardless of their input, so bpf_func_id is all that's necessary
> to make an accurate determination. However, map_lookup_elem only
> acquires when operating on certain MAP_TYPEs, so commit 64d85290d79c
> ("bpf: Allow bpf_map_lookup_elem for SOCKMAP and SOCKHASH") added the
> may_be check.
>
> Any helper which always acquires a reference should pass both
> may_be_acquire_function and is_acquire_function checks. Recently-added
> kptr_xchg passes the latter but not the former. This patch resolves this
> discrepancy and does some refactoring such that the list of functions
> which always acquire is in one place so future updates are in sync.
>
Thanks for the fix.
I actually didn't add this on purpose, because the reason for using
the may_be_acquire_function (in check_refcount_ok) doesn't apply to
kptr_xchg, but maybe that was a poor choice on my part. I'm actually
not sure of the need for may_be_acquire_function, and
check_refcount_ok.
Can we revisit why iit is needed? It only prevents
ARG_PTR_TO_SOCK_COMMON (which is not the only arg type that may be
refcounted) from being argument type of acquire functions. What is the
reason behind this? Should we rename arg_type_may_be_refcounted to a
less confusing name? It probably only applies to socket lookup
helpers.
> Fixes: c0a5a21c25f3 ("bpf: Allow storing referenced kptr in map")
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> ---
>
> Sent to bpf-next instead of bpf as kptr_xchg not passing
> may_be_acquire_function isn't currently breaking anything, just
> logically inconsistent.
>
> kernel/bpf/verifier.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 26e7e787c20a..df4b923e77de 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -477,13 +477,30 @@ static bool type_may_be_null(u32 type)
> return type & PTR_MAYBE_NULL;
> }
>
> +/* These functions acquire a resource that must be later released
> + * regardless of their input
> + */
> +static bool __check_function_always_acquires(enum bpf_func_id func_id)
> +{
> + switch (func_id) {
> + case BPF_FUNC_sk_lookup_tcp:
> + case BPF_FUNC_sk_lookup_udp:
> + case BPF_FUNC_skc_lookup_tcp:
> + case BPF_FUNC_ringbuf_reserve:
> + case BPF_FUNC_kptr_xchg:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static bool may_be_acquire_function(enum bpf_func_id func_id)
> {
> - return func_id == BPF_FUNC_sk_lookup_tcp ||
> - func_id == BPF_FUNC_sk_lookup_udp ||
> - func_id == BPF_FUNC_skc_lookup_tcp ||
> - func_id == BPF_FUNC_map_lookup_elem ||
> - func_id == BPF_FUNC_ringbuf_reserve;
> + /* See is_acquire_function for the conditions under which funcs
> + * not in __check_function_always_acquires acquire a resource
> + */
> + return __check_function_always_acquires(func_id) ||
> + func_id == BPF_FUNC_map_lookup_elem;
> }
>
> static bool is_acquire_function(enum bpf_func_id func_id,
> @@ -491,11 +508,7 @@ static bool is_acquire_function(enum bpf_func_id func_id,
> {
> enum bpf_map_type map_type = map ? map->map_type : BPF_MAP_TYPE_UNSPEC;
>
> - if (func_id == BPF_FUNC_sk_lookup_tcp ||
> - func_id == BPF_FUNC_sk_lookup_udp ||
> - func_id == BPF_FUNC_skc_lookup_tcp ||
> - func_id == BPF_FUNC_ringbuf_reserve ||
> - func_id == BPF_FUNC_kptr_xchg)
> + if (__check_function_always_acquires(func_id))
> return true;
>
> if (func_id == BPF_FUNC_map_lookup_elem &&
> --
> 2.30.2
>
