| Message ID | 20220621073233.53776-1-nashuiliang@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] libbpf: Cleanup the kprobe_event on failed add_kprobe_event_legacy() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
Chuang W wrote: > Before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with > livepatch"), in a scenario where livepatch and kprobe coexist on the > same function entry, the creation of kprobe_event using > add_kprobe_event_legacy() will be successful, at the same time as a > trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but > perf_event_open() will return an error because both livepatch and kprobe > use FTRACE_OPS_FL_IPMODIFY. > > With this patch, whenever an error is returned after > add_kprobe_event_legacy(), this ensures that the created kprobe_event is > cleaned. > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > --- > tools/lib/bpf/libbpf.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) I think we want to improve the commit message otherwise I'm sure we will stumble on this in the future and from just above its tricky to follow. I would suggest almost verbatim the description you gave in reply to my question. Just cut'n'pasting your text together with minor edit glue, " The legacy kprobe API (i.e. tracefs API) has two steps: 1) register_kprobe $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events This will create a trace event of mykprobe and register a disable kprobe that waits to be activated. 2) enable_kprobe 2.1) using syscall perf_event_open as the following code, perf_event_kprobe_open_legacy (file: tools/lib/bpf/libbpf.c): --- attr.type = PERF_TYPE_TRACEPOINT; pfd = syscall(__NR_perf_event_open, &attr, pid < 0 ? -1 : pid, /* pid */ pid == -1 ? 0 : -1, /* cpu */ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC); --- In the implementation code of perf_event_open, enable_kprobe() will be executed. 2.2) using shell $ echo 1 > /sys/kernel/debug/tracing/events/kprobes/mykprobe/enable As with perf_event_open, enable_kprobe() will also be executed. When using the same function XXX, kprobe and livepatch cannot coexist, that is, step 2) will return an error (ref: arm_kprobe_ftrace()), however, step 1) is ok! The new kprobe API (i.e. perf kprobe API) aggregates register_kprobe and enable_kprobe, internally fixes the issue on failed enable_kprobe. To fix: before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with livepatch"), in a scenario where livepatch and kprobe coexist on the same function entry, the creation of kprobe_event using add_kprobe_event_legacy() will be successful, at the same time as a trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but perf_event_open() will return an error because both livepatch and kprobe use FTRACE_OPS_FL_IPMODIFY. With this patch, whenever an error is returned after add_kprobe_event_legacy(), this ensures that the created kprobe_event is cleaned. " Thanks, John
On Tue, Jun 21, 2022 at 12:32 AM Chuang W <nashuiliang@gmail.com> wrote: > > Before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with > livepatch"), in a scenario where livepatch and kprobe coexist on the > same function entry, the creation of kprobe_event using > add_kprobe_event_legacy() will be successful, at the same time as a > trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but > perf_event_open() will return an error because both livepatch and kprobe > use FTRACE_OPS_FL_IPMODIFY. > > With this patch, whenever an error is returned after > add_kprobe_event_legacy(), this ensures that the created kprobe_event is > cleaned. > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > --- This part is good, but I think there are few error paths in bpf_program__attach_kprobe_opts() itself that would need to call remove_kprobe_event_legacy() explicitly as well, no? > tools/lib/bpf/libbpf.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 0781fae58a06..d0a36350e22a 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10809,10 +10809,11 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, > } > type = determine_kprobe_perf_type_legacy(probe_name, retprobe); > if (type < 0) { > + err = type; > pr_warn("failed to determine legacy kprobe event id for '%s+0x%zx': %s\n", > kfunc_name, offset, > - libbpf_strerror_r(type, errmsg, sizeof(errmsg))); > - return type; > + libbpf_strerror_r(err, errmsg, sizeof(errmsg))); > + goto clear_kprobe_event; > } > attr.size = sizeof(attr); > attr.config = type; > @@ -10826,9 +10827,14 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, > err = -errno; > pr_warn("legacy kprobe perf_event_open() failed: %s\n", > libbpf_strerror_r(err, errmsg, sizeof(errmsg))); > - return err; > + goto clear_kprobe_event; > } > return pfd; > + > +clear_kprobe_event: > + /* Clear the newly added kprobe_event */ > + remove_kprobe_event_legacy(probe_name, retprobe); > + return err; > } > > struct bpf_link * > -- > 2.34.1 >
Hi, John. Thanks, I will resubmit V3 soon, and supplement the commit information. On Thu, Jun 23, 2022 at 11:37 AM John Fastabend <john.fastabend@gmail.com> wrote: > > Chuang W wrote: > > Before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with > > livepatch"), in a scenario where livepatch and kprobe coexist on the > > same function entry, the creation of kprobe_event using > > add_kprobe_event_legacy() will be successful, at the same time as a > > trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but > > perf_event_open() will return an error because both livepatch and kprobe > > use FTRACE_OPS_FL_IPMODIFY. > > > > With this patch, whenever an error is returned after > > add_kprobe_event_legacy(), this ensures that the created kprobe_event is > > cleaned. > > > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > > --- > > tools/lib/bpf/libbpf.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > I think we want to improve the commit message otherwise I'm sure we will > stumble on this in the future and from just above its tricky to follow. > I would suggest almost verbatim the description you gave in reply to > my question. Just cut'n'pasting your text together with minor edit > glue, > > " > The legacy kprobe API (i.e. tracefs API) has two steps: > > 1) register_kprobe > > $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events > > This will create a trace event of mykprobe and register a disable > kprobe that waits to be activated. > > 2) enable_kprobe > > 2.1) using syscall perf_event_open as the following code, > perf_event_kprobe_open_legacy (file: tools/lib/bpf/libbpf.c): > --- > attr.type = PERF_TYPE_TRACEPOINT; > pfd = syscall(__NR_perf_event_open, &attr, > pid < 0 ? -1 : pid, /* pid */ > pid == -1 ? 0 : -1, /* cpu */ > -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC); > --- > > In the implementation code of perf_event_open, enable_kprobe() will be executed. > > 2.2) using shell > > $ echo 1 > /sys/kernel/debug/tracing/events/kprobes/mykprobe/enable > > As with perf_event_open, enable_kprobe() will also be executed. > > When using the same function XXX, kprobe and livepatch cannot coexist, > that is, step 2) will return an error (ref: arm_kprobe_ftrace()), > however, step 1) is ok! The new kprobe API (i.e. perf kprobe API) > aggregates register_kprobe and enable_kprobe, internally fixes the > issue on failed enable_kprobe. > > To fix: before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with > livepatch"), in a scenario where livepatch and kprobe coexist on the > same function entry, the creation of kprobe_event using > add_kprobe_event_legacy() will be successful, at the same time as a > trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but > perf_event_open() will return an error because both livepatch and kprobe > use FTRACE_OPS_FL_IPMODIFY. > > With this patch, whenever an error is returned after > add_kprobe_event_legacy(), this ensures that the created kprobe_event is > cleaned. > " > > Thanks, > John
Hi, Andrii, Oh, yes. I verified that when bpf_program__attach_kprobe_opts() fails, the kprobe_event did not clean up. I will resubmit V3 soon, and fix it. Thanks. On Thu, Jun 23, 2022 at 12:03 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Jun 21, 2022 at 12:32 AM Chuang W <nashuiliang@gmail.com> wrote: > > > > Before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with > > livepatch"), in a scenario where livepatch and kprobe coexist on the > > same function entry, the creation of kprobe_event using > > add_kprobe_event_legacy() will be successful, at the same time as a > > trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but > > perf_event_open() will return an error because both livepatch and kprobe > > use FTRACE_OPS_FL_IPMODIFY. > > > > With this patch, whenever an error is returned after > > add_kprobe_event_legacy(), this ensures that the created kprobe_event is > > cleaned. > > > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > > --- > > This part is good, but I think there are few error paths in > bpf_program__attach_kprobe_opts() itself that would need to call > remove_kprobe_event_legacy() explicitly as well, no? > > > tools/lib/bpf/libbpf.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 0781fae58a06..d0a36350e22a 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -10809,10 +10809,11 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, > > } > > type = determine_kprobe_perf_type_legacy(probe_name, retprobe); > > if (type < 0) { > > + err = type; > > pr_warn("failed to determine legacy kprobe event id for '%s+0x%zx': %s\n", > > kfunc_name, offset, > > - libbpf_strerror_r(type, errmsg, sizeof(errmsg))); > > - return type; > > + libbpf_strerror_r(err, errmsg, sizeof(errmsg))); > > + goto clear_kprobe_event; > > } > > attr.size = sizeof(attr); > > attr.config = type; > > @@ -10826,9 +10827,14 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, > > err = -errno; > > pr_warn("legacy kprobe perf_event_open() failed: %s\n", > > libbpf_strerror_r(err, errmsg, sizeof(errmsg))); > > - return err; > > + goto clear_kprobe_event; > > } > > return pfd; > > + > > +clear_kprobe_event: > > + /* Clear the newly added kprobe_event */ > > + remove_kprobe_event_legacy(probe_name, retprobe); > > + return err; > > } > > > > struct bpf_link * > > -- > > 2.34.1 > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 0781fae58a06..d0a36350e22a 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10809,10 +10809,11 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, } type = determine_kprobe_perf_type_legacy(probe_name, retprobe); if (type < 0) { + err = type; pr_warn("failed to determine legacy kprobe event id for '%s+0x%zx': %s\n", kfunc_name, offset, - libbpf_strerror_r(type, errmsg, sizeof(errmsg))); - return type; + libbpf_strerror_r(err, errmsg, sizeof(errmsg))); + goto clear_kprobe_event; } attr.size = sizeof(attr); attr.config = type; @@ -10826,9 +10827,14 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe, err = -errno; pr_warn("legacy kprobe perf_event_open() failed: %s\n", libbpf_strerror_r(err, errmsg, sizeof(errmsg))); - return err; + goto clear_kprobe_event; } return pfd; + +clear_kprobe_event: + /* Clear the newly added kprobe_event */ + remove_kprobe_event_legacy(probe_name, retprobe); + return err; } struct bpf_link *
Before the 0bc11ed5ab60 commit ("kprobes: Allow kprobes coexist with livepatch"), in a scenario where livepatch and kprobe coexist on the same function entry, the creation of kprobe_event using add_kprobe_event_legacy() will be successful, at the same time as a trace event (e.g. /debugfs/tracing/events/kprobe/XX) will exist, but perf_event_open() will return an error because both livepatch and kprobe use FTRACE_OPS_FL_IPMODIFY. With this patch, whenever an error is returned after add_kprobe_event_legacy(), this ensures that the created kprobe_event is cleaned. Signed-off-by: Chuang W <nashuiliang@gmail.com> Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> --- tools/lib/bpf/libbpf.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)