On Sat, Nov 9, 2024 at 2:52 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> + err = acquire_lock_state(env, env->insn_idx, REF_TYPE_LOCK, reg->id, ptr);
> + if (err < 0) {
> + verbose(env, "Failed to acquire lock state\n");
> + return err;
> + }
> + /* It is not safe to allow multiple bpf_spin_lock calls, so
> + * disallow them until this lock has been unlocked.
> + */
> + cur->active_locks++;
One more thing as suggested earlier...
pls move active_locks++ into acquire_lock_state().
> } else {
> void *ptr;
>
> @@ -7786,20 +7856,18 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno,
> else
> ptr = btf;
>
> - if (!cur->active_lock.ptr) {
> + if (!cur->active_locks) {
> verbose(env, "bpf_spin_unlock without taking a lock\n");
> return -EINVAL;
> }
> - if (cur->active_lock.ptr != ptr ||
> - cur->active_lock.id != reg->id) {
> +
> + if (release_lock_state(cur_func(env), REF_TYPE_LOCK, reg->id, ptr)) {
> verbose(env, "bpf_spin_unlock of different lock\n");
> return -EINVAL;
> }
>
> invalidate_non_owning_refs(env);
> -
> - cur->active_lock.ptr = NULL;
> - cur->active_lock.id = 0;
> + cur->active_locks--;
And similar with decrement.
imo it's cleaner this way.
The rest looks good.
Patch 2 is a nice cleanup. Thanks!
