| Message ID | 20220626031301.60390-1-nashuiliang@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| 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 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
On Sat, Jun 25, 2022 at 8:13 PM 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/XXX) will exist, but > perf_event_open() will return an error because both livepatch and kprobe > use FTRACE_OPS_FL_IPMODIFY. As follows: > > 1) add a livepatch > > $ insmod livepatch-XXX.ko > > 2) add a kprobe using tracefs API (i.e. add_kprobe_event_legacy) > > $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events > > 3) enable this kprobe (i.e. sys_perf_event_open) > > This will return an error, -EBUSY. > > On Andrii Nakryiko's comment, few error paths in > bpf_program__attach_kprobe_opts() which should need to call > remove_kprobe_event_legacy(). > > With this patch, whenever an error is returned after > add_kprobe_event_legacy() or bpf_program__attach_perf_event_opts(), this > ensures that the created kprobe_event is cleaned. > > Signed-off-by: Chuang W <nashuiliang@gmail.com> Is this your full name? Signed-off-by is required to have a full name of a person, please update if it's not > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > --- > V2->v3: > - add detail commits > - call remove_kprobe_event_legacy() on failed bpf_program__attach_perf_event_opts() > > tools/lib/bpf/libbpf.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 49e359cd34df..038b0cb3313f 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10811,10 +10811,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; > @@ -10828,9 +10829,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 legacy kprobe_event */ > + remove_kprobe_event_legacy(probe_name, retprobe); > + return err; > } > this part looks good > struct bpf_link * > @@ -10899,6 +10905,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, > > return link; > err_out: > + /* Clear the newly added legacy kprobe_event */ > + if (legacy) > + remove_kprobe_event_legacy(legacy_probe, retprobe); this one will call remove_kprobe_event_legacy() even if we failed to create that kprobe_event in the first place. So let's maybe add err_clean_legacy: if (legacy) remove_kprobe_event_legacy(legacy_probe, retprobe); before err_out: and goto there if we fail to attach (but not if we fail to create pfd)? Also, looking through libbpf code, I realized that we have exactly the same problem for uprobes, so please add same fixed to perf_event_uprobe_open_legacy and attach_uprobe_opts. Thanks! > free(legacy_probe); > return libbpf_err_ptr(err); > } > -- > 2.34.1 >
Hi Andrii, On Tue, Jun 28, 2022 at 5:27 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sat, Jun 25, 2022 at 8:13 PM 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/XXX) will exist, but > > perf_event_open() will return an error because both livepatch and kprobe > > use FTRACE_OPS_FL_IPMODIFY. As follows: > > > > 1) add a livepatch > > > > $ insmod livepatch-XXX.ko > > > > 2) add a kprobe using tracefs API (i.e. add_kprobe_event_legacy) > > > > $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events > > > > 3) enable this kprobe (i.e. sys_perf_event_open) > > > > This will return an error, -EBUSY. > > > > On Andrii Nakryiko's comment, few error paths in > > bpf_program__attach_kprobe_opts() which should need to call > > remove_kprobe_event_legacy(). > > > > With this patch, whenever an error is returned after > > add_kprobe_event_legacy() or bpf_program__attach_perf_event_opts(), this > > ensures that the created kprobe_event is cleaned. > > > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > > Is this your full name? Signed-off-by is required to have a full name > of a person, please update if it's not > > > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > > --- > > V2->v3: > > - add detail commits > > - call remove_kprobe_event_legacy() on failed bpf_program__attach_perf_event_opts() > > > > tools/lib/bpf/libbpf.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 49e359cd34df..038b0cb3313f 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -10811,10 +10811,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; > > @@ -10828,9 +10829,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 legacy kprobe_event */ > > + remove_kprobe_event_legacy(probe_name, retprobe); > > + return err; > > } > > > > this part looks good > > > > struct bpf_link * > > @@ -10899,6 +10905,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, > > > > return link; > > err_out: > > + /* Clear the newly added legacy kprobe_event */ > > + if (legacy) > > + remove_kprobe_event_legacy(legacy_probe, retprobe); > > this one will call remove_kprobe_event_legacy() even if we failed to > create that kprobe_event in the first place. So let's maybe add > > err_clean_legacy: > if (legacy) > remove_kprobe_event_legacy(legacy_probe, retprobe); > > before err_out: and goto there if we fail to attach (but not if we > fail to create pfd)? > Nice, I will modify it. > > Also, looking through libbpf code, I realized that we have exactly the > same problem for uprobes, so please add same fixed to > perf_event_uprobe_open_legacy and attach_uprobe_opts. Thanks! > Oh, yes. I also noticed this problem for uprobes, I was planning to submit a patch for uprobes. Do you think I should submit another patch for uprobes or combine kprobes and uprobes into one? Thanks, > > > > free(legacy_probe); > > return libbpf_err_ptr(err); > > } > > -- > > 2.34.1 > >
On Mon, Jun 27, 2022 at 6:51 PM Chuang W <nashuiliang@gmail.com> wrote: > > Hi Andrii, > > On Tue, Jun 28, 2022 at 5:27 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Sat, Jun 25, 2022 at 8:13 PM 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/XXX) will exist, but > > > perf_event_open() will return an error because both livepatch and kprobe > > > use FTRACE_OPS_FL_IPMODIFY. As follows: > > > > > > 1) add a livepatch > > > > > > $ insmod livepatch-XXX.ko > > > > > > 2) add a kprobe using tracefs API (i.e. add_kprobe_event_legacy) > > > > > > $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events > > > > > > 3) enable this kprobe (i.e. sys_perf_event_open) > > > > > > This will return an error, -EBUSY. > > > > > > On Andrii Nakryiko's comment, few error paths in > > > bpf_program__attach_kprobe_opts() which should need to call > > > remove_kprobe_event_legacy(). > > > > > > With this patch, whenever an error is returned after > > > add_kprobe_event_legacy() or bpf_program__attach_perf_event_opts(), this > > > ensures that the created kprobe_event is cleaned. > > > > > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > > > > Is this your full name? Signed-off-by is required to have a full name > > of a person, please update if it's not > > > > > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > > > --- > > > V2->v3: > > > - add detail commits > > > - call remove_kprobe_event_legacy() on failed bpf_program__attach_perf_event_opts() > > > > > > tools/lib/bpf/libbpf.c | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index 49e359cd34df..038b0cb3313f 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -10811,10 +10811,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; > > > @@ -10828,9 +10829,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 legacy kprobe_event */ > > > + remove_kprobe_event_legacy(probe_name, retprobe); > > > + return err; > > > } > > > > > > > this part looks good > > > > > > > struct bpf_link * > > > @@ -10899,6 +10905,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, > > > > > > return link; > > > err_out: > > > + /* Clear the newly added legacy kprobe_event */ > > > + if (legacy) > > > + remove_kprobe_event_legacy(legacy_probe, retprobe); > > > > this one will call remove_kprobe_event_legacy() even if we failed to > > create that kprobe_event in the first place. So let's maybe add > > > > err_clean_legacy: > > if (legacy) > > remove_kprobe_event_legacy(legacy_probe, retprobe); > > > > before err_out: and goto there if we fail to attach (but not if we > > fail to create pfd)? > > > > Nice, I will modify it. > > > > > Also, looking through libbpf code, I realized that we have exactly the > > same problem for uprobes, so please add same fixed to > > perf_event_uprobe_open_legacy and attach_uprobe_opts. Thanks! > > > > Oh, yes. I also noticed this problem for uprobes, I was planning to > submit a patch for uprobes. > Do you think I should submit another patch for uprobes or combine > kprobes and uprobes into one? > two separate patches make more sense, but send them as a patch series? > Thanks, > > > > > > > free(legacy_probe); > > > return libbpf_err_ptr(err); > > > } > > > -- > > > 2.34.1 > > >
Ok. Thanks, On Tue, Jun 28, 2022 at 10:35 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Jun 27, 2022 at 6:51 PM Chuang W <nashuiliang@gmail.com> wrote: > > > > Hi Andrii, > > > > On Tue, Jun 28, 2022 at 5:27 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Sat, Jun 25, 2022 at 8:13 PM 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/XXX) will exist, but > > > > perf_event_open() will return an error because both livepatch and kprobe > > > > use FTRACE_OPS_FL_IPMODIFY. As follows: > > > > > > > > 1) add a livepatch > > > > > > > > $ insmod livepatch-XXX.ko > > > > > > > > 2) add a kprobe using tracefs API (i.e. add_kprobe_event_legacy) > > > > > > > > $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events > > > > > > > > 3) enable this kprobe (i.e. sys_perf_event_open) > > > > > > > > This will return an error, -EBUSY. > > > > > > > > On Andrii Nakryiko's comment, few error paths in > > > > bpf_program__attach_kprobe_opts() which should need to call > > > > remove_kprobe_event_legacy(). > > > > > > > > With this patch, whenever an error is returned after > > > > add_kprobe_event_legacy() or bpf_program__attach_perf_event_opts(), this > > > > ensures that the created kprobe_event is cleaned. > > > > > > > > Signed-off-by: Chuang W <nashuiliang@gmail.com> > > > > > > Is this your full name? Signed-off-by is required to have a full name > > > of a person, please update if it's not > > > > > > > Signed-off-by: Jingren Zhou <zhoujingren@didiglobal.com> > > > > --- > > > > V2->v3: > > > > - add detail commits > > > > - call remove_kprobe_event_legacy() on failed bpf_program__attach_perf_event_opts() > > > > > > > > tools/lib/bpf/libbpf.c | 15 ++++++++++++--- > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > > index 49e359cd34df..038b0cb3313f 100644 > > > > --- a/tools/lib/bpf/libbpf.c > > > > +++ b/tools/lib/bpf/libbpf.c > > > > @@ -10811,10 +10811,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; > > > > @@ -10828,9 +10829,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 legacy kprobe_event */ > > > > + remove_kprobe_event_legacy(probe_name, retprobe); > > > > + return err; > > > > } > > > > > > > > > > this part looks good > > > > > > > > > > struct bpf_link * > > > > @@ -10899,6 +10905,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, > > > > > > > > return link; > > > > err_out: > > > > + /* Clear the newly added legacy kprobe_event */ > > > > + if (legacy) > > > > + remove_kprobe_event_legacy(legacy_probe, retprobe); > > > > > > this one will call remove_kprobe_event_legacy() even if we failed to > > > create that kprobe_event in the first place. So let's maybe add > > > > > > err_clean_legacy: > > > if (legacy) > > > remove_kprobe_event_legacy(legacy_probe, retprobe); > > > > > > before err_out: and goto there if we fail to attach (but not if we > > > fail to create pfd)? > > > > > > > Nice, I will modify it. > > > > > > > > Also, looking through libbpf code, I realized that we have exactly the > > > same problem for uprobes, so please add same fixed to > > > perf_event_uprobe_open_legacy and attach_uprobe_opts. Thanks! > > > > > > > Oh, yes. I also noticed this problem for uprobes, I was planning to > > submit a patch for uprobes. > > Do you think I should submit another patch for uprobes or combine > > kprobes and uprobes into one? > > > > two separate patches make more sense, but send them as a patch series? > > > Thanks, > > > > > > > > > > free(legacy_probe); > > > > return libbpf_err_ptr(err); > > > > } > > > > -- > > > > 2.34.1 > > > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 49e359cd34df..038b0cb3313f 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10811,10 +10811,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; @@ -10828,9 +10829,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 legacy kprobe_event */ + remove_kprobe_event_legacy(probe_name, retprobe); + return err; } struct bpf_link * @@ -10899,6 +10905,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog, return link; err_out: + /* Clear the newly added legacy kprobe_event */ + if (legacy) + remove_kprobe_event_legacy(legacy_probe, retprobe); free(legacy_probe); return libbpf_err_ptr(err); }
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/XXX) will exist, but perf_event_open() will return an error because both livepatch and kprobe use FTRACE_OPS_FL_IPMODIFY. As follows: 1) add a livepatch $ insmod livepatch-XXX.ko 2) add a kprobe using tracefs API (i.e. add_kprobe_event_legacy) $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events 3) enable this kprobe (i.e. sys_perf_event_open) This will return an error, -EBUSY. On Andrii Nakryiko's comment, few error paths in bpf_program__attach_kprobe_opts() which should need to call remove_kprobe_event_legacy(). With this patch, whenever an error is returned after add_kprobe_event_legacy() or bpf_program__attach_perf_event_opts(), 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> --- V2->v3: - add detail commits - call remove_kprobe_event_legacy() on failed bpf_program__attach_perf_event_opts() tools/lib/bpf/libbpf.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)