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@xxxxxxxxx/
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@xxxxxx>
> ---
> 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)
