On 12/29/22 11:54 AM, David Vernet wrote:
> On Wed, Dec 28, 2022 at 07:56:00PM -0800, Alexei Starovoitov wrote:
>> On Sat, Dec 17, 2022 at 12:24:55AM -0800, Dave Marchevsky wrote:
>>> This patch introduces non-owning reference semantics to the verifier,
>>> specifically linked_list API kfunc handling. release_on_unlock logic for
>>> refs is refactored - with small functional changes - to implement these
>>> semantics, and bpf_list_push_{front,back} are migrated to use them.
>>>
>>> When a list node is pushed to a list, the program still has a pointer to
>>> the node:
>>>
>>> n = bpf_obj_new(typeof(*n));
>>>
>>> bpf_spin_lock(&l);
>>> bpf_list_push_back(&l, n);
>>> /* n still points to the just-added node */
>>> bpf_spin_unlock(&l);
>>>
>>> What the verifier considers n to be after the push, and thus what can be
>>> done with n, are changed by this patch.
>>>
>>> Common properties both before/after this patch:
>>> * After push, n is only a valid reference to the node until end of
>>> critical section
>>> * After push, n cannot be pushed to any list
>>> * After push, the program can read the node's fields using n
>>
>> correct.
>>
>>> Before:
>>> * After push, n retains the ref_obj_id which it received on
>>> bpf_obj_new, but the associated bpf_reference_state's
>>> release_on_unlock field is set to true
>>> * release_on_unlock field and associated logic is used to implement
>>> "n is only a valid ref until end of critical section"
>>> * After push, n cannot be written to, the node must be removed from
>>> the list before writing to its fields
>>> * After push, n is marked PTR_UNTRUSTED
>>
>> yep
>>
>>> After:
>>> * After push, n's ref is released and ref_obj_id set to 0. The
>>> bpf_reg_state's non_owning_ref_lock struct is populated with the
>>> currently active lock
>>> * non_owning_ref_lock and logic is used to implement "n is only a
>>> valid ref until end of critical section"
>>> * n can be written to (except for special fields e.g. bpf_list_node,
>>> timer, ...)
>>> * No special type flag is added to n after push
>>
>> yep.
>> Great summary.
>>
>>> Summary of specific implementation changes to achieve the above:
>>>
>>> * release_on_unlock field, ref_set_release_on_unlock helper, and logic
>>> to "release on unlock" based on that field are removed
>>
>> +1
>>
>>> * The anonymous active_lock struct used by bpf_verifier_state is
>>> pulled out into a named struct bpf_active_lock.
>> ...
>>> * A non_owning_ref_lock field of type bpf_active_lock is added to
>>> bpf_reg_state's PTR_TO_BTF_ID union
>>
>> not great. see below.
>>
>>> * Helpers are added to use non_owning_ref_lock to implement non-owning
>>> ref semantics as described above
>>> * invalidate_non_owning_refs - helper to clobber all non-owning refs
>>> matching a particular bpf_active_lock identity. Replaces
>>> release_on_unlock logic in process_spin_lock.
>>
>> +1
>>
>>> * ref_set_non_owning_lock - set non_owning_ref_lock for a reg based
>>> on current verifier state
>>
>> +1
>>
>>> * ref_convert_owning_non_owning - convert owning reference w/
>>> specified ref_obj_id to non-owning references. Setup
>>> non_owning_ref_lock for each reg with that ref_obj_id and 0 out
>>> its ref_obj_id
>>
>> +1
>>
>>> * New KF_RELEASE_NON_OWN flag is added, to be used in conjunction with
>>> KF_RELEASE to indicate that the release arg reg should be converted
>>> to non-owning ref
>>> * Plain KF_RELEASE would clobber all regs with ref_obj_id matching
>>> the release arg reg's. KF_RELEASE_NON_OWN's logic triggers first -
>>> doing ref_convert_owning_non_owning on the ref first, which
>>> prevents the regs from being clobbered by 0ing out their
>>> ref_obj_ids. The bpf_reference_state itself is still released via
>>> release_reference as a result of the KF_RELEASE flag.
>>> * KF_RELEASE | KF_RELEASE_NON_OWN are added to
>>> bpf_list_push_{front,back}
>>
>> And this bit is confusing and not generalizable.
>
> +1 on both counts. If we want to make it generalizable, I think the only
> way to do would be to generalize it across different graph map types.
> For example, to have kfunc flags like KF_GRAPH_INSERT and
> KF_GRAPH_REMOVE which signal to the verifier that "for this graph-type
> map which has a spin-lock associated with its root node that I expect to
> be held, I've {inserted, removed} the node {to, from} the graph, so
> adjust the refcnt / pointer type accordingly and then clean up when the
> lock is dropped."
>
> I don't see any reason to add kfunc flags for that though, as the fact
> that the pointer in question refers to a node that has a root node that
> has a lock associated with it is already itself a special-case scenario.
> I think we should just special-case these kfuncs in the verifier as
> "graph-type" kfuncs in some static global array(s). That's probably
> less error prone anyways, and I don't see the typical kfunc writer ever
> needing to do this.
>
re: "generalizable" and "why add a kfunc flag at all", I addressed that in
a side-reply to the msg which you're replying to here [0].
But to address your specific points:
"the fact that the pointer in question refers to a node that has a root node
that has a lock associated with it is already itself a special-case scenario"
Are you trying to say here that because the arg is of a special type, special
behavior should be tied to that arg type instead of the function? If so, that's
addressed in [0]. A function with KF_RELEASE_NON_OWN semantics certainly does
need args of a certain type in order to do its thing, but the semantic is really
a function-level thing. If we can tag the function with it, then later check arg
regs, that's preferable to checking kfunc id while processing arg regs, as the
latter conflates "the arg is of this special type so the function should do X"
with "the function does X so it must have an arg with this special type".
[0]: https://lore.kernel.org/bpf/9763aed7-0284-e400-b4dc-ed01718d8e1e@xxxxxxxx/
>> As David noticed in his reply KF_RELEASE_NON_OWN is not a great name.
>> It's hard to come up with a good name and it won't be generic anyway.
>> The ref_convert_owning_non_owning has to be applied to a specific arg.
>> The function itself is not KF_RELEASE in the current definition of it.
>> The combination of KF_RELEASE|KF_RELEASE_NON_OWN is something new
>> that should have been generic, but doesn't really work this way.
>> In the next patches rbtree_root/node still has to have all the custom
>> logic.
>> KF_RELEASE_NON_OWN by itself is a nonsensical flag.
>
> IMO if a flag doesn't make any sense on its own, or even possibly if it
> needs to be mutually exclusive with one or more other flags, it is
> probably never a correct building block. Even KF_TRUSTED_ARGS doesn't
> really make sense, as it's redundant if KF_RCU is specified. This is
> fine though, as IIUC our long-term plan is to get rid of KF_TRUSTED_ARGS
> and make it the default behavior for all kfuncs (not trying to hijack
> this thread with a tangential discussion about KF_TRUSTED_ARGS, just
> using this as an opportunity to point out something to keep in mind as
> we continue to add kfunc flags down the road).
>
I'm fine with making KF_RELEASE_NON_OWN not depend on KF_RELEASE. Addressed in
[0] above.
>> Only combination of KF_RELEASE|KF_RELEASE_NON_OWN sort-of kinda makes
>> sense, but still hard to understand what releases what.
>
> I agree and I think this is an important point. IMO it is a worse
> tradeoff to try to generalize this by complicating the definition of a
> reference than it is to keep the refcounting APIs straightforward and
> well defined. As a basic building block, having an owning refcount
> should mean one thing: that the object will not be destroyed and is safe
> to dereference. When you start mixing in these graph-specific notions of
> references meaning different things in specific contexts, it compromises
> that and makes the API significantly less usable and extensible.
>
"Generalize" was the wrong word for me to use here. Addressed in [0] above.
Regarding polluting the meaning of "reference": owning and non-owning references
are intentionally scoped to graph datastructures only, and have well-defined and
documented meaning in that context. Elsewhere in the verifier "reference",
"owning refcount", etc are not well-defined as folks have been adding whatever
semantics they need to get their stuff working for some time. Scoping these
new concepts to graph datastructures only is my attempt at making progress
without adding to that confusion.
> For example, at some point we may decide to add something like a
> kptr_weak_ref which would function exactly like an std::weak_ptr, except
> of course that it would wrap a kptr_ref instead of an std::shared_ptr.
> IMO something like that is a more natural and generalizable building
> block that cleanly complements refcounting as it exists today.
>
Any functionality that implements the desired semantics for rbtree / linked_list
is fine with me. If it's a superset of what I'm adding here, happy to migrate.
If changes to rbtree/linked_list APIs are needed to make such migration
possible, luckily it's all unstable kptr/kfunc, so that better future state
isn't blocked by these semantics / implementation.
All this next-gen datastructure work has been an exercise in YAGNI and scope
reduction. Luckily since it's all unstable API we're not backing ourselves
into any corners by doing so.
re "std::weak_ptr" idea more specifically - despite obvious similarities to
some rust or cpp concepts, I've been intentionally avoiding trying to sell this
work as such or copying semantics wholesale. Better to focus on the specific
things needed to move forward and avoid starting big-scope arguments like
"should we just add std::shared_ptr semantics?" "should the verifier be doing
'borrow checking' similar to rust, and if so to what extent". Don't get me
wrong, I'd find such discussions interesting, but a YAGNI approach where such
functionality is gradually added in response to concrete usecases will likely
save much contentious back-and-forth.
>> More below.
>>
>>> After these changes, linked_list's "release on unlock" logic continues
>>> to function as before, except for the semantic differences noted above.
>>> The patch immediately following this one makes minor changes to
>>> linked_list selftests to account for the differing behavior.
>>>
>>> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
>>> ---
>>> include/linux/bpf.h | 1 +
>>> include/linux/bpf_verifier.h | 39 ++++-----
>>> include/linux/btf.h | 17 ++--
>>> kernel/bpf/helpers.c | 4 +-
>>> kernel/bpf/verifier.c | 164 ++++++++++++++++++++++++-----------
>>> 5 files changed, 146 insertions(+), 79 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index 3de24cfb7a3d..f71571bf6adc 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -180,6 +180,7 @@ enum btf_field_type {
>>> BPF_KPTR = BPF_KPTR_UNREF | BPF_KPTR_REF,
>>> BPF_LIST_HEAD = (1 << 4),
>>> BPF_LIST_NODE = (1 << 5),
>>> + BPF_GRAPH_NODE_OR_ROOT = BPF_LIST_NODE | BPF_LIST_HEAD,
>
> Can you update the rest of the elements here to keep common indentation?
>
Ack
>>> };
>>>
>>> struct btf_field_kptr {
>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>> index 53d175cbaa02..cb417ffbbb84 100644
>>> --- a/include/linux/bpf_verifier.h
>>> +++ b/include/linux/bpf_verifier.h
>>> @@ -43,6 +43,22 @@ enum bpf_reg_liveness {
>>> REG_LIVE_DONE = 0x8, /* liveness won't be updating this register anymore */
>>> };
>>>
>>> +/* For every reg representing a map value or allocated object pointer,
>>> + * we consider the tuple of (ptr, id) for them to be unique in verifier
>>> + * context and conside them to not alias each other for the purposes of
>>> + * tracking lock state.
>>> + */
>>> +struct bpf_active_lock {
>>> + /* This can either be reg->map_ptr or reg->btf. If ptr is NULL,
>>> + * there's no active lock held, and other fields have no
>>> + * meaning. If non-NULL, it indicates that a lock is held and
>>> + * id member has the reg->id of the register which can be >= 0.
>>> + */
>>> + void *ptr;
>>> + /* This will be reg->id */
>>> + u32 id;
>>> +};
>>> +
>>> struct bpf_reg_state {
>>> /* Ordering of fields matters. See states_equal() */
>>> enum bpf_reg_type type;
>>> @@ -68,6 +84,7 @@ struct bpf_reg_state {
>>> struct {
>>> struct btf *btf;
>>> u32 btf_id;
>>> + struct bpf_active_lock non_owning_ref_lock;
>>
>> In your other email you argue that pointer should be enough.
>> I suspect that won't be correct.
>> See fixes that Andrii did in states_equal() and regsafe().
>> In particular:
>> if (!!old->active_lock.id != !!cur->active_lock.id)
>> return false;
>>
>> if (old->active_lock.id &&
>> !check_ids(old->active_lock.id, cur->active_lock.id, env->idmap_scratch))
>> return false;
>>
>> We have to do the comparison of this new ID via idmap as well.
>>
>> I think introduction of struct bpf_active_lock and addition of it
>> to bpf_reg_state is overkill.
>> Here we can add 'u32 non_own_ref_obj_id;' only and compare it via idmap in regsafe().
>> I'm guessing you didn't like my 'active_lock_id' suggestion. Fine.
>> non_own_ref_obj_id would match existing ref_obj_id at least.
>>
>>> };
>>>
>>> u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
>>> @@ -223,11 +240,6 @@ struct bpf_reference_state {
>>> * exiting a callback function.
>>> */
>>> int callback_ref;
>>> - /* Mark the reference state to release the registers sharing the same id
>>> - * on bpf_spin_unlock (for nodes that we will lose ownership to but are
>>> - * safe to access inside the critical section).
>>> - */
>>> - bool release_on_unlock;
>>> };
>>>
>>> /* state of the program:
>>> @@ -328,21 +340,8 @@ struct bpf_verifier_state {
>>> u32 branches;
>>> u32 insn_idx;
>>> u32 curframe;
>>> - /* For every reg representing a map value or allocated object pointer,
>>> - * we consider the tuple of (ptr, id) for them to be unique in verifier
>>> - * context and conside them to not alias each other for the purposes of
>>> - * tracking lock state.
>>> - */
>>> - struct {
>>> - /* This can either be reg->map_ptr or reg->btf. If ptr is NULL,
>>> - * there's no active lock held, and other fields have no
>>> - * meaning. If non-NULL, it indicates that a lock is held and
>>> - * id member has the reg->id of the register which can be >= 0.
>>> - */
>>> - void *ptr;
>>> - /* This will be reg->id */
>>> - u32 id;
>>> - } active_lock;
>>
>> I would keep it as-is.
>>
>>> +
>>> + struct bpf_active_lock active_lock;
>>> bool speculative;
>>> bool active_rcu_lock;
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index 5f628f323442..8aee3f7f4248 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -15,10 +15,10 @@
>>> #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>>>
>>> /* These need to be macros, as the expressions are used in assembler input */
>>> -#define KF_ACQUIRE (1 << 0) /* kfunc is an acquire function */
>>> -#define KF_RELEASE (1 << 1) /* kfunc is a release function */
>>> -#define KF_RET_NULL (1 << 2) /* kfunc returns a pointer that may be NULL */
>>> -#define KF_KPTR_GET (1 << 3) /* kfunc returns reference to a kptr */
>>> +#define KF_ACQUIRE (1 << 0) /* kfunc is an acquire function */
>>> +#define KF_RELEASE (1 << 1) /* kfunc is a release function */
>>> +#define KF_RET_NULL (1 << 2) /* kfunc returns a pointer that may be NULL */
>>> +#define KF_KPTR_GET (1 << 3) /* kfunc returns reference to a kptr */
>>> /* Trusted arguments are those which are guaranteed to be valid when passed to
>>> * the kfunc. It is used to enforce that pointers obtained from either acquire
>>> * kfuncs, or from the main kernel on a tracepoint or struct_ops callback
>>> @@ -67,10 +67,11 @@
>>> * return 0;
>>> * }
>>> */
>>> -#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
>>> -#define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */
>>> -#define KF_DESTRUCTIVE (1 << 6) /* kfunc performs destructive actions */
>>> -#define KF_RCU (1 << 7) /* kfunc only takes rcu pointer arguments */
>>> +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
>>> +#define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */
>>> +#define KF_DESTRUCTIVE (1 << 6) /* kfunc performs destructive actions */
>>> +#define KF_RCU (1 << 7) /* kfunc only takes rcu pointer arguments */
>>> +#define KF_RELEASE_NON_OWN (1 << 8) /* kfunc converts its referenced arg into non-owning ref */
>>
>> No need for this flag.
>>
>>> /*
>>> * Return the name of the passed struct, if exists, or halt the build if for
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index af30c6cbd65d..e041409779c3 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -2049,8 +2049,8 @@ BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
>>> #endif
>>> BTF_ID_FLAGS(func, bpf_obj_new_impl, KF_ACQUIRE | KF_RET_NULL)
>>> BTF_ID_FLAGS(func, bpf_obj_drop_impl, KF_RELEASE)
>>> -BTF_ID_FLAGS(func, bpf_list_push_front)
>>> -BTF_ID_FLAGS(func, bpf_list_push_back)
>>> +BTF_ID_FLAGS(func, bpf_list_push_front, KF_RELEASE | KF_RELEASE_NON_OWN)
>>> +BTF_ID_FLAGS(func, bpf_list_push_back, KF_RELEASE | KF_RELEASE_NON_OWN)
>>
>> No need for this.
>>
>>> BTF_ID_FLAGS(func, bpf_list_pop_front, KF_ACQUIRE | KF_RET_NULL)
>>> BTF_ID_FLAGS(func, bpf_list_pop_back, KF_ACQUIRE | KF_RET_NULL)
>>> BTF_ID_FLAGS(func, bpf_task_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 824e2242eae5..84b0660e2a76 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -190,6 +190,10 @@ struct bpf_verifier_stack_elem {
>>>
>>> static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx);
>>> static int release_reference(struct bpf_verifier_env *env, int ref_obj_id);
>>> +static void invalidate_non_owning_refs(struct bpf_verifier_env *env,
>>> + struct bpf_active_lock *lock);
>>> +static int ref_set_non_owning_lock(struct bpf_verifier_env *env,
>>> + struct bpf_reg_state *reg);
>>>
>>> static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux)
>>> {
>>> @@ -931,6 +935,9 @@ static void print_verifier_state(struct bpf_verifier_env *env,
>>> verbose_a("id=%d", reg->id);
>>> if (reg->ref_obj_id)
>>> verbose_a("ref_obj_id=%d", reg->ref_obj_id);
>>> + if (reg->non_owning_ref_lock.ptr)
>>> + verbose_a("non_own_id=(%p,%d)", reg->non_owning_ref_lock.ptr,
>>> + reg->non_owning_ref_lock.id);
>>> if (t != SCALAR_VALUE)
>>> verbose_a("off=%d", reg->off);
>>> if (type_is_pkt_pointer(t))
>>> @@ -4820,7 +4827,8 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>>> return -EACCES;
>>> }
>>>
>>> - if (type_is_alloc(reg->type) && !reg->ref_obj_id) {
>>> + if (type_is_alloc(reg->type) && !reg->ref_obj_id &&
>>> + !reg->non_owning_ref_lock.ptr) {
>>> verbose(env, "verifier internal error: ref_obj_id for allocated object must be non-zero\n");
>>> return -EFAULT;
>>> }
>>> @@ -5778,9 +5786,7 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno,
>>> cur->active_lock.ptr = btf;
>>> cur->active_lock.id = reg->id;
>>> } else {
>>> - struct bpf_func_state *fstate = cur_func(env);
>>> void *ptr;
>>> - int i;
>>>
>>> if (map)
>>> ptr = map;
>>> @@ -5796,25 +5802,11 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno,
>>> verbose(env, "bpf_spin_unlock of different lock\n");
>>> return -EINVAL;
>>> }
>>> - cur->active_lock.ptr = NULL;
>>> - cur->active_lock.id = 0;
>>>
>>> - for (i = fstate->acquired_refs - 1; i >= 0; i--) {
>>> - int err;
>>> + invalidate_non_owning_refs(env, &cur->active_lock);
>>
>> +1
>>
>>> - /* Complain on error because this reference state cannot
>>> - * be freed before this point, as bpf_spin_lock critical
>>> - * section does not allow functions that release the
>>> - * allocated object immediately.
>>> - */
>>> - if (!fstate->refs[i].release_on_unlock)
>>> - continue;
>>> - err = release_reference(env, fstate->refs[i].id);
>>> - if (err) {
>>> - verbose(env, "failed to release release_on_unlock reference");
>>> - return err;
>>> - }
>>> - }
>>> + cur->active_lock.ptr = NULL;
>>> + cur->active_lock.id = 0;
>>
>> +1
>>
>>> }
>>> return 0;
>>> }
>>> @@ -6273,6 +6265,23 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>>> return 0;
>>> }
>>>
>>> +static struct btf_field *
>>> +reg_find_field_offset(const struct bpf_reg_state *reg, s32 off, u32 fields)
>>> +{
>>> + struct btf_field *field;
>>> + struct btf_record *rec;
>>> +
>>> + rec = reg_btf_record(reg);
>>> + if (!reg)
>>> + return NULL;
>>> +
>>> + field = btf_record_find(rec, off, fields);
>>> + if (!field)
>>> + return NULL;
>>> +
>>> + return field;
>>> +}
>>
>> Doesn't look like that this helper is really necessary.
>>
>>> +
>>> int check_func_arg_reg_off(struct bpf_verifier_env *env,
>>> const struct bpf_reg_state *reg, int regno,
>>> enum bpf_arg_type arg_type)
>>> @@ -6294,6 +6303,18 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
>>> */
>>> if (arg_type_is_dynptr(arg_type) && type == PTR_TO_STACK)
>>> return 0;
>>> +
>>> + if (type == (PTR_TO_BTF_ID | MEM_ALLOC) && reg->off) {
>>> + if (reg_find_field_offset(reg, reg->off, BPF_GRAPH_NODE_OR_ROOT))
>>> + return __check_ptr_off_reg(env, reg, regno, true);
>>> +
>>> + verbose(env, "R%d must have zero offset when passed to release func\n",
>>> + regno);
>>> + verbose(env, "No graph node or root found at R%d type:%s off:%d\n", regno,
>>> + kernel_type_name(reg->btf, reg->btf_id), reg->off);
>>> + return -EINVAL;
>>> + }
>>
>> This bit is only necessary if we mark push_list as KF_RELEASE.
>> Just don't add this mark and drop above.
>>
>>> +
>>> /* Doing check_ptr_off_reg check for the offset will catch this
>>> * because fixed_off_ok is false, but checking here allows us
>>> * to give the user a better error message.
>>> @@ -7055,6 +7076,20 @@ static int release_reference(struct bpf_verifier_env *env,
>>> return 0;
>>> }
>>>
>>> +static void invalidate_non_owning_refs(struct bpf_verifier_env *env,
>>> + struct bpf_active_lock *lock)
>>> +{
>>> + struct bpf_func_state *unused;
>>> + struct bpf_reg_state *reg;
>>> +
>>> + bpf_for_each_reg_in_vstate(env->cur_state, unused, reg, ({
>>> + if (reg->non_owning_ref_lock.ptr &&
>>> + reg->non_owning_ref_lock.ptr == lock->ptr &&
>>> + reg->non_owning_ref_lock.id == lock->id)
>>
>> I think the lock.ptr = lock->ptr comparison is unnecessary to invalidate things.
>> We're under active spin_lock here. All regs were checked earlier and id keeps incrementing.
>> So we can just do 'u32 non_own_ref_obj_id'.
>>
>>> + __mark_reg_unknown(env, reg);
>>> + }));
>>> +}
>>> +
>>> static void clear_caller_saved_regs(struct bpf_verifier_env *env,
>>> struct bpf_reg_state *regs)
>>> {
>>> @@ -8266,6 +8301,11 @@ static bool is_kfunc_release(struct bpf_kfunc_call_arg_meta *meta)
>>> return meta->kfunc_flags & KF_RELEASE;
>>> }
>>>
>>> +static bool is_kfunc_release_non_own(struct bpf_kfunc_call_arg_meta *meta)
>>> +{
>>> + return meta->kfunc_flags & KF_RELEASE_NON_OWN;
>>> +}
>>> +
>>
>> No need.
>>
>>> static bool is_kfunc_trusted_args(struct bpf_kfunc_call_arg_meta *meta)
>>> {
>>> return meta->kfunc_flags & KF_TRUSTED_ARGS;
>>> @@ -8651,38 +8691,55 @@ static int process_kf_arg_ptr_to_kptr(struct bpf_verifier_env *env,
>>> return 0;
>>> }
>>>
>>> -static int ref_set_release_on_unlock(struct bpf_verifier_env *env, u32 ref_obj_id)
>>> +static int ref_set_non_owning_lock(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
>>> {
>>> - struct bpf_func_state *state = cur_func(env);
>>> + struct bpf_verifier_state *state = env->cur_state;
>>> +
>>> + if (!state->active_lock.ptr) {
>>> + verbose(env, "verifier internal error: ref_set_non_owning_lock w/o active lock\n");
>>> + return -EFAULT;
>>> + }
>>> +
>>> + if (reg->non_owning_ref_lock.ptr) {
>>> + verbose(env, "verifier internal error: non_owning_ref_lock already set\n");
>>> + return -EFAULT;
>>> + }
>>> +
>>> + reg->non_owning_ref_lock.id = state->active_lock.id;
>>> + reg->non_owning_ref_lock.ptr = state->active_lock.ptr;
>>> + return 0;
>>> +}
>>> +
>>> +static int ref_convert_owning_non_owning(struct bpf_verifier_env *env, u32 ref_obj_id)
>>> +{
>>> + struct bpf_func_state *state, *unused;
>>> struct bpf_reg_state *reg;
>>> int i;
>>>
>>> - /* bpf_spin_lock only allows calling list_push and list_pop, no BPF
>>> - * subprogs, no global functions. This means that the references would
>>> - * not be released inside the critical section but they may be added to
>>> - * the reference state, and the acquired_refs are never copied out for a
>>> - * different frame as BPF to BPF calls don't work in bpf_spin_lock
>>> - * critical sections.
>>> - */
>>> + state = cur_func(env);
>>> +
>>> if (!ref_obj_id) {
>>> - verbose(env, "verifier internal error: ref_obj_id is zero for release_on_unlock\n");
>>> + verbose(env, "verifier internal error: ref_obj_id is zero for "
>>> + "owning -> non-owning conversion\n");
>>> return -EFAULT;
>>> }
>>> +
>>> for (i = 0; i < state->acquired_refs; i++) {
>>> - if (state->refs[i].id == ref_obj_id) {
>>> - if (state->refs[i].release_on_unlock) {
>>> - verbose(env, "verifier internal error: expected false release_on_unlock");
>>> - return -EFAULT;
>>> + if (state->refs[i].id != ref_obj_id)
>>> + continue;
>>> +
>>> + /* Clear ref_obj_id here so release_reference doesn't clobber
>>> + * the whole reg
>>> + */
>>> + bpf_for_each_reg_in_vstate(env->cur_state, unused, reg, ({
>>> + if (reg->ref_obj_id == ref_obj_id) {
>>> + reg->ref_obj_id = 0;
>>> + ref_set_non_owning_lock(env, reg);
>>
>> +1 except ref_set_... name doesn't quite fit. reg_set_... is more accurate, no?
>> and probably reg_set_non_own_ref_obj_id() ?
>> Or just open code it?
>>
>>> }
>>> - state->refs[i].release_on_unlock = true;
>>> - /* Now mark everyone sharing same ref_obj_id as untrusted */
>>> - bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
>>> - if (reg->ref_obj_id == ref_obj_id)
>>> - reg->type |= PTR_UNTRUSTED;
>>> - }));
>>> - return 0;
>>> - }
>>> + }));
>>> + return 0;
>>> }
>>> +
>>> verbose(env, "verifier internal error: ref state missing for ref_obj_id\n");
>>> return -EFAULT;
>>> }
>>> @@ -8817,7 +8874,6 @@ static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env,
>>> {
>>> const struct btf_type *et, *t;
>>> struct btf_field *field;
>>> - struct btf_record *rec;
>>> u32 list_node_off;
>>>
>>> if (meta->btf != btf_vmlinux ||
>>> @@ -8834,9 +8890,8 @@ static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env,
>>> return -EINVAL;
>>> }
>>>
>>> - rec = reg_btf_record(reg);
>>> list_node_off = reg->off + reg->var_off.value;
>>> - field = btf_record_find(rec, list_node_off, BPF_LIST_NODE);
>>> + field = reg_find_field_offset(reg, list_node_off, BPF_LIST_NODE);
>>> if (!field || field->offset != list_node_off) {
>>> verbose(env, "bpf_list_node not found at offset=%u\n", list_node_off);
>>> return -EINVAL;
>>> @@ -8861,8 +8916,8 @@ static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env,
>>> btf_name_by_offset(field->list_head.btf, et->name_off));
>>> return -EINVAL;
>>> }
>>> - /* Set arg#1 for expiration after unlock */
>>> - return ref_set_release_on_unlock(env, reg->ref_obj_id);
>>> +
>>> + return 0;
>>
>> and here we come to the main point.
>> Can you just call
>> ref_convert_owning_non_owning(env, reg->ref_obj_id) and release_reference() here?
>> Everything will be so much simpler, no?
>>
>>> }
>>>
>>> static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta)
>>> @@ -9132,11 +9187,11 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>> int *insn_idx_p)
>>> {
>>> const struct btf_type *t, *func, *func_proto, *ptr_type;
>>> + u32 i, nargs, func_id, ptr_type_id, release_ref_obj_id;
>>> struct bpf_reg_state *regs = cur_regs(env);
>>> const char *func_name, *ptr_type_name;
>>> bool sleepable, rcu_lock, rcu_unlock;
>>> struct bpf_kfunc_call_arg_meta meta;
>>> - u32 i, nargs, func_id, ptr_type_id;
>>> int err, insn_idx = *insn_idx_p;
>>> const struct btf_param *args;
>>> const struct btf_type *ret_t;
>>> @@ -9223,7 +9278,18 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>> * PTR_TO_BTF_ID in bpf_kfunc_arg_meta, do the release now.
>>> */
>>> if (meta.release_regno) {
>>> - err = release_reference(env, regs[meta.release_regno].ref_obj_id);
>>> + err = 0;
>>> + release_ref_obj_id = regs[meta.release_regno].ref_obj_id;
>>> +
>>> + if (is_kfunc_release_non_own(&meta))
>>> + err = ref_convert_owning_non_owning(env, release_ref_obj_id);
>>> + if (err) {
>>> + verbose(env, "kfunc %s#%d conversion of owning ref to non-owning failed\n",
>>> + func_name, func_id);
>>> + return err;
>>> + }
>>> +
>>> + err = release_reference(env, release_ref_obj_id);
>>
>> and this bit won't be needed.
>> and no need to guess in patch 1 which arg has to be released and converted to non_own.
>>
>>> if (err) {
>>> verbose(env, "kfunc %s#%d reference has not been acquired before\n",
>>> func_name, func_id);
>>> --
>>> 2.30.2
>>>
