On Thu, 2023-04-20 at 18:49 -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> Florian and Eduard reported hard dead lock:
> [ 58.433327] _raw_spin_lock_irqsave+0x40/0x50
> [ 58.433334] btf_put+0x43/0x90
> [ 58.433338] bpf_find_btf_id+0x157/0x240
> [ 58.433353] btf_parse_fields+0x921/0x11c0
>
> This happens since btf->refcount can be 1 at the time of btf_put() and
> btf_put() will call btf_free_id() which will try to grab btf_idr_lock
> and will dead lock.
> Avoid the issue by doing btf_put() without locking.
>
> Reported-by: Florian Westphal <fw@xxxxxxxxx>
> Reported-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> Fixes: 3d78417b60fb ("bpf: Add bpf_btf_find_by_name_kind() helper.")
> Fixes: 1e89106da253 ("bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn().")
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
I applied the patch from Dave, that fixes address computation
in bpf_refcount_acquire_impl() and tested this patch using the
following reproducing script (to obtain a race between test module
unload and bpf_find_btf_id():
for j in $(seq 1 100);
do echo ">>>> $j <<<<";
for i in $(seq 1 4); do (./test_progs --allow=refcounted_kptr &); done;
sleep 1;
done
W/o this patch I see dead locks, with this patch I don't see dead locks.
Tested-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> kernel/bpf/btf.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index a0887ee44e89..7db4ec125fbd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -577,8 +577,8 @@ static s32 bpf_find_btf_id(const char *name, u32 kind, struct btf **btf_p)
> *btf_p = btf;
> return ret;
> }
> - spin_lock_bh(&btf_idr_lock);
> btf_put(btf);
> + spin_lock_bh(&btf_idr_lock);
> }
> spin_unlock_bh(&btf_idr_lock);
> return ret;
> @@ -8354,12 +8354,10 @@ bpf_core_find_cands(struct bpf_core_ctx *ctx, u32 local_type_id)
> btf_get(mod_btf);
> spin_unlock_bh(&btf_idr_lock);
> cands = bpf_core_add_cands(cands, mod_btf, btf_nr_types(main_btf));
> - if (IS_ERR(cands)) {
> - btf_put(mod_btf);
> + btf_put(mod_btf);
> + if (IS_ERR(cands))
> return ERR_CAST(cands);
> - }
> spin_lock_bh(&btf_idr_lock);
> - btf_put(mod_btf);
> }
> spin_unlock_bh(&btf_idr_lock);
> /* cands is a pointer to kmalloced memory here if cands->cnt > 0
