* [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach
@ 2021-01-11 19:16 Jiri Olsa
2021-01-11 20:03 ` Toke Høiland-Jørgensen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-01-11 19:16 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Toke Høiland-Jørgensen
The bpf_tracing_prog_attach error path calls bpf_prog_put
on prog, which causes refcount underflow when it's called
from link_create function.
link_create
prog = bpf_prog_get <-- get
...
tracing_bpf_link_attach(prog..
bpf_tracing_prog_attach(prog..
out_put_prog:
bpf_prog_put(prog); <-- put
if (ret < 0)
bpf_prog_put(prog); <-- put
Removing bpf_prog_put call from bpf_tracing_prog_attach
and making sure its callers call it instead.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/bpf/syscall.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c3bb03c8371f..e5999d86c76e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2712,7 +2712,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
out_put_prog:
if (tgt_prog_fd && tgt_prog)
bpf_prog_put(tgt_prog);
- bpf_prog_put(prog);
return err;
}
@@ -2825,7 +2824,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
tp_name = prog->aux->attach_func_name;
break;
}
- return bpf_tracing_prog_attach(prog, 0, 0);
+ err = bpf_tracing_prog_attach(prog, 0, 0);
+ if (err >= 0)
+ return err;
+ goto out_put_prog;
case BPF_PROG_TYPE_RAW_TRACEPOINT:
case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
if (strncpy_from_user(buf,
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach 2021-01-11 19:16 [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach Jiri Olsa @ 2021-01-11 20:03 ` Toke Høiland-Jørgensen 2021-01-11 20:34 ` Andrii Nakryiko 2021-01-11 23:40 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: Toke Høiland-Jørgensen @ 2021-01-11 20:03 UTC (permalink / raw) To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh Jiri Olsa <jolsa@kernel.org> writes: > The bpf_tracing_prog_attach error path calls bpf_prog_put > on prog, which causes refcount underflow when it's called > from link_create function. > > link_create > prog = bpf_prog_get <-- get > ... > tracing_bpf_link_attach(prog.. > bpf_tracing_prog_attach(prog.. > out_put_prog: > bpf_prog_put(prog); <-- put > > if (ret < 0) > bpf_prog_put(prog); <-- put > > Removing bpf_prog_put call from bpf_tracing_prog_attach > and making sure its callers call it instead. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach 2021-01-11 19:16 [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach Jiri Olsa 2021-01-11 20:03 ` Toke Høiland-Jørgensen @ 2021-01-11 20:34 ` Andrii Nakryiko 2021-01-11 21:17 ` Jiri Olsa 2021-01-11 23:40 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2021-01-11 20:34 UTC (permalink / raw) To: Jiri Olsa Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Toke Høiland-Jørgensen On Mon, Jan 11, 2021 at 11:18 AM Jiri Olsa <jolsa@kernel.org> wrote: > > The bpf_tracing_prog_attach error path calls bpf_prog_put > on prog, which causes refcount underflow when it's called > from link_create function. > > link_create > prog = bpf_prog_get <-- get > ... > tracing_bpf_link_attach(prog.. > bpf_tracing_prog_attach(prog.. > out_put_prog: > bpf_prog_put(prog); <-- put > > if (ret < 0) > bpf_prog_put(prog); <-- put > > Removing bpf_prog_put call from bpf_tracing_prog_attach > and making sure its callers call it instead. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- I also double-checked all other attach functions called from link_create, they all seem to be fine and don't put prog on failure, so this should be the only needed fix. Also, missing: Fixes: 4a1e7c0c63e0 ("bpf: Support attaching freplace programs to multiple attach points") Acked-by: Andrii Nakryiko <andrii@kernel.org> > kernel/bpf/syscall.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index c3bb03c8371f..e5999d86c76e 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2712,7 +2712,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > out_put_prog: > if (tgt_prog_fd && tgt_prog) > bpf_prog_put(tgt_prog); > - bpf_prog_put(prog); > return err; > } > > @@ -2825,7 +2824,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) > tp_name = prog->aux->attach_func_name; > break; > } > - return bpf_tracing_prog_attach(prog, 0, 0); > + err = bpf_tracing_prog_attach(prog, 0, 0); > + if (err >= 0) > + return err; > + goto out_put_prog; > case BPF_PROG_TYPE_RAW_TRACEPOINT: > case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: > if (strncpy_from_user(buf, > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach 2021-01-11 20:34 ` Andrii Nakryiko @ 2021-01-11 21:17 ` Jiri Olsa 2021-01-11 21:26 ` Andrii Nakryiko 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2021-01-11 21:17 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Toke Høiland-Jørgensen On Mon, Jan 11, 2021 at 12:34:48PM -0800, Andrii Nakryiko wrote: > On Mon, Jan 11, 2021 at 11:18 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > The bpf_tracing_prog_attach error path calls bpf_prog_put > > on prog, which causes refcount underflow when it's called > > from link_create function. > > > > link_create > > prog = bpf_prog_get <-- get > > ... > > tracing_bpf_link_attach(prog.. > > bpf_tracing_prog_attach(prog.. > > out_put_prog: > > bpf_prog_put(prog); <-- put > > > > if (ret < 0) > > bpf_prog_put(prog); <-- put > > > > Removing bpf_prog_put call from bpf_tracing_prog_attach > > and making sure its callers call it instead. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > I also double-checked all other attach functions called from > link_create, they all seem to be fine and don't put prog on failure, > so this should be the only needed fix. Also, missing: it'd be easier to spot this if we use refcount_t instead of the atomic64_t, I replaced it for this refcount and got nice console warning for this bug then I saw: 85192dbf4de0 bpf: Convert bpf_prog refcnt to atomic64_t so I guess we need something like refcount64_t first jirka > > Fixes: 4a1e7c0c63e0 ("bpf: Support attaching freplace programs to > multiple attach points") > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > kernel/bpf/syscall.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index c3bb03c8371f..e5999d86c76e 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -2712,7 +2712,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > > out_put_prog: > > if (tgt_prog_fd && tgt_prog) > > bpf_prog_put(tgt_prog); > > - bpf_prog_put(prog); > > return err; > > } > > > > @@ -2825,7 +2824,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) > > tp_name = prog->aux->attach_func_name; > > break; > > } > > - return bpf_tracing_prog_attach(prog, 0, 0); > > + err = bpf_tracing_prog_attach(prog, 0, 0); > > + if (err >= 0) > > + return err; > > + goto out_put_prog; > > case BPF_PROG_TYPE_RAW_TRACEPOINT: > > case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: > > if (strncpy_from_user(buf, > > -- > > 2.26.2 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach 2021-01-11 21:17 ` Jiri Olsa @ 2021-01-11 21:26 ` Andrii Nakryiko 0 siblings, 0 replies; 6+ messages in thread From: Andrii Nakryiko @ 2021-01-11 21:26 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Toke Høiland-Jørgensen On Mon, Jan 11, 2021 at 1:17 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Mon, Jan 11, 2021 at 12:34:48PM -0800, Andrii Nakryiko wrote: > > On Mon, Jan 11, 2021 at 11:18 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > The bpf_tracing_prog_attach error path calls bpf_prog_put > > > on prog, which causes refcount underflow when it's called > > > from link_create function. > > > > > > link_create > > > prog = bpf_prog_get <-- get > > > ... > > > tracing_bpf_link_attach(prog.. > > > bpf_tracing_prog_attach(prog.. > > > out_put_prog: > > > bpf_prog_put(prog); <-- put > > > > > > if (ret < 0) > > > bpf_prog_put(prog); <-- put > > > > > > Removing bpf_prog_put call from bpf_tracing_prog_attach > > > and making sure its callers call it instead. > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > > I also double-checked all other attach functions called from > > link_create, they all seem to be fine and don't put prog on failure, > > so this should be the only needed fix. Also, missing: > > it'd be easier to spot this if we use refcount_t instead of the atomic64_t, > I replaced it for this refcount and got nice console warning for this bug > > then I saw: > 85192dbf4de0 bpf: Convert bpf_prog refcnt to atomic64_t > > so I guess we need something like refcount64_t first Having a non-failing refcount simplifies code quite a lot. I was having a problem having to deal with potential refcount failure during mmap()'ing, where it's impossible to communicate failure back to the kernel. So if atomic64_t would never fail, but would generate a warning on underflow, then yeah, it would be an improvement. > > jirka > > > > > Fixes: 4a1e7c0c63e0 ("bpf: Support attaching freplace programs to > > multiple attach points") > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > kernel/bpf/syscall.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > index c3bb03c8371f..e5999d86c76e 100644 > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -2712,7 +2712,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > > > out_put_prog: > > > if (tgt_prog_fd && tgt_prog) > > > bpf_prog_put(tgt_prog); > > > - bpf_prog_put(prog); > > > return err; > > > } > > > > > > @@ -2825,7 +2824,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) > > > tp_name = prog->aux->attach_func_name; > > > break; > > > } > > > - return bpf_tracing_prog_attach(prog, 0, 0); > > > + err = bpf_tracing_prog_attach(prog, 0, 0); > > > + if (err >= 0) > > > + return err; > > > + goto out_put_prog; > > > case BPF_PROG_TYPE_RAW_TRACEPOINT: > > > case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: > > > if (strncpy_from_user(buf, > > > -- > > > 2.26.2 > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach 2021-01-11 19:16 [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach Jiri Olsa 2021-01-11 20:03 ` Toke Høiland-Jørgensen 2021-01-11 20:34 ` Andrii Nakryiko @ 2021-01-11 23:40 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2021-01-11 23:40 UTC (permalink / raw) To: Jiri Olsa Cc: ast, daniel, andriin, netdev, bpf, kafai, songliubraving, yhs, john.fastabend, kpsingh, toke Hello: This patch was applied to bpf/bpf.git (refs/heads/master): On Mon, 11 Jan 2021 20:16:50 +0100 you wrote: > The bpf_tracing_prog_attach error path calls bpf_prog_put > on prog, which causes refcount underflow when it's called > from link_create function. > > link_create > prog = bpf_prog_get <-- get > ... > tracing_bpf_link_attach(prog.. > bpf_tracing_prog_attach(prog.. > out_put_prog: > bpf_prog_put(prog); <-- put > > [...] Here is the summary with links: - bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach https://git.kernel.org/bpf/bpf/c/5541075a348b You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-12 0:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-11 19:16 [PATCH] bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach Jiri Olsa 2021-01-11 20:03 ` Toke Høiland-Jørgensen 2021-01-11 20:34 ` Andrii Nakryiko 2021-01-11 21:17 ` Jiri Olsa 2021-01-11 21:26 ` Andrii Nakryiko 2021-01-11 23:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.