| Message ID | 20230421014901.70908-1-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | acf1c3d68e9a31f10d92bc67ad4673cdae5e8d92 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Fix race between btf_put and btf_idr walk. | expand |
On Thu, 2023-04-20 at 18:49 -0700, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@kernel.org> > > 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@strlen.de> > Reported-by: Eduard Zingerman <eddyz87@gmail.com> > 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@kernel.org> 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@gmail.com> > --- > 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
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Thu, 20 Apr 2023 18:49:01 -0700 you wrote: > From: Alexei Starovoitov <ast@kernel.org> > > 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 > > [...] Here is the summary with links: - [bpf-next] bpf: Fix race between btf_put and btf_idr walk. https://git.kernel.org/bpf/bpf-next/c/acf1c3d68e9a You are awesome, thank you!
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