* [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event()
@ 2022-06-26 3:13 Chuang W
2022-06-27 21:27 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Chuang W @ 2022-06-26 3:13 UTC (permalink / raw)
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, netdev, bpf, linux-kernel, Masami Hiramatsu, Chuang W,
Jingren Zhou
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(-)
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);
}
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event()
2022-06-26 3:13 [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event() Chuang W
@ 2022-06-27 21:27 ` Andrii Nakryiko
2022-06-28 1:51 ` Chuang W
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2022-06-27 21:27 UTC (permalink / raw)
To: Chuang W
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, open list, Masami Hiramatsu,
Jingren Zhou
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event()
2022-06-27 21:27 ` Andrii Nakryiko
@ 2022-06-28 1:51 ` Chuang W
2022-06-28 2:35 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Chuang W @ 2022-06-28 1:51 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, open list, Masami Hiramatsu,
Jingren Zhou
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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event()
2022-06-28 1:51 ` Chuang W
@ 2022-06-28 2:35 ` Andrii Nakryiko
2022-06-28 2:43 ` Chuang W
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2022-06-28 2:35 UTC (permalink / raw)
To: Chuang W
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, open list, Masami Hiramatsu,
Jingren Zhou
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
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event()
2022-06-28 2:35 ` Andrii Nakryiko
@ 2022-06-28 2:43 ` Chuang W
0 siblings, 0 replies; 5+ messages in thread
From: Chuang W @ 2022-06-28 2:43 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, open list, Masami Hiramatsu,
Jingren Zhou
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
> > > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-28 2:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-26 3:13 [PATCH v3] libbpf: Cleanup the legacy kprobe_event on failed add/attach_event() Chuang W
2022-06-27 21:27 ` Andrii Nakryiko
2022-06-28 1:51 ` Chuang W
2022-06-28 2:35 ` Andrii Nakryiko
2022-06-28 2:43 ` Chuang W
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®