Re: [RFC PATCH] bpf: Prevent recursive deadlocks in BPF programs attached to spin lock helpers using fentry/ fexit

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Jan 24, 2024 at 10:43:32AM -0500, Siddharth Chintamaneni wrote:
> While we were working on some experiments with BPF trampoline, we came
> across a deadlock scenario that could happen.
> 
> A deadlock happens when two nested BPF programs tries to acquire the
> same lock i.e, If a BPF program is attached using fexit to
> bpf_spin_lock or using a fentry to bpf_spin_unlock, and it then
> attempts to acquire the same lock as the previous BPF program, a
> deadlock situation arises.
> 
> Here is an example:
> 
> SEC(fentry/bpf_spin_unlock)
> int fentry_2{
>   bpf_spin_lock(&x->lock);
>   bpf_spin_unlock(&x->lock);
> }
> 
> SEC(fentry/xxx)
> int fentry_1{
>   bpf_spin_lock(&x->lock);
>   bpf_spin_unlock(&x->lock);
> }

hi,
looks like valid issue, could you add selftest for that?

I wonder we could restrict just programs that use bpf_spin_lock/bpf_spin_unlock
helpers? I'm not sure there's any useful use case for tracing spin lock helpers,
but I think we should at least try this before we deny it completely

> 
> To prevent these cases, a simple fix could be adding these helpers to
> denylist in the verifier. This fix will prevent the BPF programs from
> being loaded by the verifier.
> 
> previously, a similar solution was proposed to prevent recursion.
> https://lore.kernel.org/lkml/20230417154737.12740-2-laoar.shao@gmail.com/

the difference is that __rcu_read_lock/__rcu_read_unlock are called unconditionally
(always) when executing bpf tracing probe, the problem you described above is only
for programs calling spin lock helpers (on same spin lock)

> 
> Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@vt.edu>
> ---
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 65f598694d55..8f1834f27f81 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20617,6 +20617,10 @@ BTF_ID(func, preempt_count_sub)
>  BTF_ID(func, __rcu_read_lock)
>  BTF_ID(func, __rcu_read_unlock)
>  #endif
> +#if defined(CONFIG_DYNAMIC_FTRACE)

why the CONFIG_DYNAMIC_FTRACE dependency?

jirka

> +BTF_ID(func, bpf_spin_lock)
> +BTF_ID(func, bpf_spin_unlock)
> +#endif
>  BTF_SET_END(btf_id_deny)