On 11/3/22 3:10 PM, Kumar Kartikeya Dwivedi wrote:
> Global variables reside in maps accessible using direct_value_addr
> callbacks, so giving each load instruction's rewrite a unique reg->id
> disallows us from holding locks which are global.
>
> This is not great, so refactor the active_spin_lock into two separate
> fields, active_spin_lock_ptr and active_spin_lock_id, which is generic
> enough to allow it for global variables, map lookups, and local kptr
> registers at the same time.
>
> Held vs non-held is indicated by active_spin_lock_ptr, which stores the
> reg->map_ptr or reg->btf pointer of the register used for locking spin
> lock. But the active_spin_lock_id also needs to be compared to ensure
> whether bpf_spin_unlock is for the same register.
>
> Next, pseudo load instructions are not given a unique reg->id, as they
> are doing lookup for the same map value (max_entries is never greater
> than 1).
>
> Essentially, we consider that the tuple of (active_spin_lock_ptr,
> active_spin_lock_id) will always be unique for any kind of argument to
> bpf_spin_{lock,unlock}.
>
> Note that this can be extended in the future to also remember offset
> used for locking, so that we can introduce multiple bpf_spin_lock fields
> in the same allocation.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> include/linux/bpf_verifier.h | 3 ++-
> kernel/bpf/verifier.c | 39 +++++++++++++++++++++++++-----------
> 2 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 1a32baa78ce2..bb71c59f21f6 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -323,7 +323,8 @@ struct bpf_verifier_state {
> u32 branches;
> u32 insn_idx;
> u32 curframe;
> - u32 active_spin_lock;
> + void *active_spin_lock_ptr;
> + u32 active_spin_lock_id;
> bool speculative;
Back in first RFC of this series we talked about turning this "spin lock
identity" concept into a proper struct [0]. But to save you the click:
Dave:
"""
It would be good to make this "(lock_ptr, lock_id) is identifier for lock"
concept more concrete by grouping these fields in a struct w/ type enum + union,
or something similar. Will make it more obvious that they should be used / set
together.
But if you'd prefer to keep it as two fields, active_spin_lock_ptr is a
confusing name. In the future with no context as to what that field is, I'd
assume that it holds a pointer to a spin_lock instead of a "spin lock identity
pointer".
"""
Kumar:
"""
That's a good point.
I'm thinking
struct active_lock {
void *id_ptr;
u32 offset;
u32 reg_id;
};
How does that look?
"""
I didn't get back to you, but I think that looks reasonable, and "this can be
extended in the future to also remember offset used for locking" in this
patch summary supports the desire to group up those fields. (I agree that
offset isn't necessary for now, though).
[0]: https://lore.kernel.org/bpf/311eb0d0-777a-4240-9fa0-59134344f051@xxxxxx/
