| Message ID | 20210328112629.339266-2-jolsa@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,bpf-next,1/4] bpf: Allow trampoline re-attach | expand |
> On Mar 28, 2021, at 4:26 AM, Jiri Olsa <jolsa@kernel.org> wrote: > > Currently we don't allow re-attaching of trampolines. Once > it's detached, it can't be re-attach even when the program > is still loaded. > > Adding the possibility to re-attach the loaded tracing > kernel program. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> LGTM. Acked-by: Song Liu <songliubraving@fb.com> [...]
Jiri Olsa <jolsa@kernel.org> writes: > Currently we don't allow re-attaching of trampolines. Once > it's detached, it can't be re-attach even when the program > is still loaded. > > Adding the possibility to re-attach the loaded tracing > kernel program. Hmm, yeah, didn't really consider this case when I added the original disallow. But don't see why not, so (with one nit below): Acked-by: Toke Høiland-Jørgensen <toke@redhat.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/bpf/syscall.c | 25 +++++++++++++++++++------ > kernel/bpf/trampoline.c | 2 +- > 2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 9603de81811a..e14926b2e95a 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2645,14 +2645,27 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > * target_btf_id using the link_create API. > * > * - if tgt_prog == NULL when this function was called using the old > - * raw_tracepoint_open API, and we need a target from prog->aux > - * > - * The combination of no saved target in prog->aux, and no target > - * specified on load is illegal, and we reject that here. > + * raw_tracepoint_open API, and we need a target from prog->aux > + * > + * The combination of no saved target in prog->aux, and no target > + * specified on is legal only for tracing programs re-attach, rest > + * is illegal, and we reject that here. > */ > if (!prog->aux->dst_trampoline && !tgt_prog) { > - err = -ENOENT; > - goto out_unlock; > + /* > + * Allow re-attach for tracing programs, if it's currently > + * linked, bpf_trampoline_link_prog will fail. > + */ > + if (prog->type != BPF_PROG_TYPE_TRACING) { > + err = -ENOENT; > + goto out_unlock; > + } > + if (!prog->aux->attach_btf) { > + err = -EINVAL; > + goto out_unlock; > + } I'm wondering about the two different return codes here. Under what circumstances will aux->attach_btf be NULL, and why is that not an ENOENT error? :) -Toke
On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote: > > if (!prog->aux->dst_trampoline && !tgt_prog) { > > - err = -ENOENT; > > - goto out_unlock; > > + /* > > + * Allow re-attach for tracing programs, if it's currently > > + * linked, bpf_trampoline_link_prog will fail. > > + */ > > + if (prog->type != BPF_PROG_TYPE_TRACING) { > > + err = -ENOENT; > > + goto out_unlock; > > + } > > + if (!prog->aux->attach_btf) { > > + err = -EINVAL; > > + goto out_unlock; > > + } > > I'm wondering about the two different return codes here. Under what > circumstances will aux->attach_btf be NULL, and why is that not an > ENOENT error? :) The feature makes sense to me as well. I don't quite see how it would get here with attach_btf == NULL. Maybe WARN_ON then? Also if we're allowing re-attach this way why exclude PROG_EXT and LSM?
On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote: > Jiri Olsa <jolsa@kernel.org> writes: > > > Currently we don't allow re-attaching of trampolines. Once > > it's detached, it can't be re-attach even when the program > > is still loaded. > > > > Adding the possibility to re-attach the loaded tracing > > kernel program. > > Hmm, yeah, didn't really consider this case when I added the original > disallow. But don't see why not, so (with one nit below): > > Acked-by: Toke Høiland-Jørgensen <toke@redhat.com> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > kernel/bpf/syscall.c | 25 +++++++++++++++++++------ > > kernel/bpf/trampoline.c | 2 +- > > 2 files changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 9603de81811a..e14926b2e95a 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -2645,14 +2645,27 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > > * target_btf_id using the link_create API. > > * > > * - if tgt_prog == NULL when this function was called using the old > > - * raw_tracepoint_open API, and we need a target from prog->aux > > - * > > - * The combination of no saved target in prog->aux, and no target > > - * specified on load is illegal, and we reject that here. > > + * raw_tracepoint_open API, and we need a target from prog->aux > > + * > > + * The combination of no saved target in prog->aux, and no target > > + * specified on is legal only for tracing programs re-attach, rest > > + * is illegal, and we reject that here. > > */ > > if (!prog->aux->dst_trampoline && !tgt_prog) { > > - err = -ENOENT; > > - goto out_unlock; > > + /* > > + * Allow re-attach for tracing programs, if it's currently > > + * linked, bpf_trampoline_link_prog will fail. > > + */ > > + if (prog->type != BPF_PROG_TYPE_TRACING) { > > + err = -ENOENT; > > + goto out_unlock; > > + } > > + if (!prog->aux->attach_btf) { > > + err = -EINVAL; > > + goto out_unlock; > > + } > > I'm wondering about the two different return codes here. Under what > circumstances will aux->attach_btf be NULL, and why is that not an > ENOENT error? :) right, that should be always there.. I'll remove it thanks, jirka
On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote: > On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote: > > > if (!prog->aux->dst_trampoline && !tgt_prog) { > > > - err = -ENOENT; > > > - goto out_unlock; > > > + /* > > > + * Allow re-attach for tracing programs, if it's currently > > > + * linked, bpf_trampoline_link_prog will fail. > > > + */ > > > + if (prog->type != BPF_PROG_TYPE_TRACING) { > > > + err = -ENOENT; > > > + goto out_unlock; > > > + } > > > + if (!prog->aux->attach_btf) { > > > + err = -EINVAL; > > > + goto out_unlock; > > > + } > > > > I'm wondering about the two different return codes here. Under what > > circumstances will aux->attach_btf be NULL, and why is that not an > > ENOENT error? :) > > The feature makes sense to me as well. > I don't quite see how it would get here with attach_btf == NULL. > Maybe WARN_ON then? right, that should be always there > Also if we're allowing re-attach this way why exclude PROG_EXT and LSM? > I was enabling just what I needed for the test, which is so far the only use case.. I'll see if I can enable that for all of them jirka
Jiri Olsa <jolsa@redhat.com> writes: > On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote: >> On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote: >> > > if (!prog->aux->dst_trampoline && !tgt_prog) { >> > > - err = -ENOENT; >> > > - goto out_unlock; >> > > + /* >> > > + * Allow re-attach for tracing programs, if it's currently >> > > + * linked, bpf_trampoline_link_prog will fail. >> > > + */ >> > > + if (prog->type != BPF_PROG_TYPE_TRACING) { >> > > + err = -ENOENT; >> > > + goto out_unlock; >> > > + } >> > > + if (!prog->aux->attach_btf) { >> > > + err = -EINVAL; >> > > + goto out_unlock; >> > > + } >> > >> > I'm wondering about the two different return codes here. Under what >> > circumstances will aux->attach_btf be NULL, and why is that not an >> > ENOENT error? :) >> >> The feature makes sense to me as well. >> I don't quite see how it would get here with attach_btf == NULL. >> Maybe WARN_ON then? > > right, that should be always there > >> Also if we're allowing re-attach this way why exclude PROG_EXT and LSM? >> > > I was enabling just what I needed for the test, which is so far > the only use case.. I'll see if I can enable that for all of them How would that work? For PROG_EXT we clear the destination on the first attach (to avoid keeping a ref on it), so re-attach can only be done with an explicit target (which already works just fine)... -Toke
On Mon, Apr 05, 2021 at 04:15:54PM +0200, Toke Høiland-Jørgensen wrote: > Jiri Olsa <jolsa@redhat.com> writes: > > > On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote: > >> On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote: > >> > > if (!prog->aux->dst_trampoline && !tgt_prog) { > >> > > - err = -ENOENT; > >> > > - goto out_unlock; > >> > > + /* > >> > > + * Allow re-attach for tracing programs, if it's currently > >> > > + * linked, bpf_trampoline_link_prog will fail. > >> > > + */ > >> > > + if (prog->type != BPF_PROG_TYPE_TRACING) { > >> > > + err = -ENOENT; > >> > > + goto out_unlock; > >> > > + } > >> > > + if (!prog->aux->attach_btf) { > >> > > + err = -EINVAL; > >> > > + goto out_unlock; > >> > > + } > >> > > >> > I'm wondering about the two different return codes here. Under what > >> > circumstances will aux->attach_btf be NULL, and why is that not an > >> > ENOENT error? :) > >> > >> The feature makes sense to me as well. > >> I don't quite see how it would get here with attach_btf == NULL. > >> Maybe WARN_ON then? > > > > right, that should be always there > > > >> Also if we're allowing re-attach this way why exclude PROG_EXT and LSM? > >> > > > > I was enabling just what I needed for the test, which is so far > > the only use case.. I'll see if I can enable that for all of them > > How would that work? For PROG_EXT we clear the destination on the first > attach (to avoid keeping a ref on it), so re-attach can only be done > with an explicit target (which already works just fine)... right, I'm just looking on it ;-) extensions already seem allow for that, I'll check LSM jirka
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 9603de81811a..e14926b2e95a 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2645,14 +2645,27 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, * target_btf_id using the link_create API. * * - if tgt_prog == NULL when this function was called using the old - * raw_tracepoint_open API, and we need a target from prog->aux - * - * The combination of no saved target in prog->aux, and no target - * specified on load is illegal, and we reject that here. + * raw_tracepoint_open API, and we need a target from prog->aux + * + * The combination of no saved target in prog->aux, and no target + * specified on is legal only for tracing programs re-attach, rest + * is illegal, and we reject that here. */ if (!prog->aux->dst_trampoline && !tgt_prog) { - err = -ENOENT; - goto out_unlock; + /* + * Allow re-attach for tracing programs, if it's currently + * linked, bpf_trampoline_link_prog will fail. + */ + if (prog->type != BPF_PROG_TYPE_TRACING) { + err = -ENOENT; + goto out_unlock; + } + if (!prog->aux->attach_btf) { + err = -EINVAL; + goto out_unlock; + } + btf_id = prog->aux->attach_btf_id; + key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id); } if (!prog->aux->dst_trampoline || diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 4aa8b52adf25..0d937c63fc22 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -467,7 +467,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr) tr->extension_prog = NULL; goto out; } - hlist_del(&prog->aux->tramp_hlist); + hlist_del_init(&prog->aux->tramp_hlist); tr->progs_cnt[kind]--; err = bpf_trampoline_update(tr); out:
Currently we don't allow re-attaching of trampolines. Once it's detached, it can't be re-attach even when the program is still loaded. Adding the possibility to re-attach the loaded tracing kernel program. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/bpf/syscall.c | 25 +++++++++++++++++++------ kernel/bpf/trampoline.c | 2 +- 2 files changed, 20 insertions(+), 7 deletions(-)