| Message ID | 20210111191650.1241578-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5541075a348b6ca6ac668653f7d2c423ae8e00b6 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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>
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 >
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 > > >
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 > > > > > >
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
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,
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(-)