* [bpf-next PATCH] bpf: libbpf, support older style kprobe load
@ 2019-10-18 14:54 John Fastabend
2019-10-18 19:44 ` Daniel Borkmann
2019-10-22 2:57 ` Andrii Nakryiko
0 siblings, 2 replies; 12+ messages in thread
From: John Fastabend @ 2019-10-18 14:54 UTC (permalink / raw)
To: andriin, ast, daniel; +Cc: netdev, bpf, john.fastabend
Following ./Documentation/trace/kprobetrace.rst add support for loading
kprobes programs on older kernels.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 73 insertions(+), 8 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fcea6988f962..12b3105d112c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
return parse_uint_from_file(file, "config:%d\n");
}
+static int use_kprobe_debugfs(const char *name,
+ uint64_t offset, int pid, bool retprobe)
+{
+ const char *file = "/sys/kernel/debug/tracing/kprobe_events";
+ int fd = open(file, O_WRONLY | O_APPEND, 0);
+ char buf[PATH_MAX];
+ int err;
+
+ if (fd < 0) {
+ pr_warning("failed open kprobe_events: %s\n",
+ strerror(errno));
+ return -errno;
+ }
+
+ snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
+ retprobe ? 'r' : 'p', name, name);
+
+ err = write(fd, buf, strlen(buf));
+ close(fd);
+ if (err < 0)
+ return -errno;
+ return 0;
+}
+
static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
uint64_t offset, int pid)
{
struct perf_event_attr attr = {};
char errmsg[STRERR_BUFSIZE];
+ uint64_t config1 = 0;
int type, pfd, err;
type = uprobe ? determine_uprobe_perf_type()
: determine_kprobe_perf_type();
if (type < 0) {
- pr_warning("failed to determine %s perf type: %s\n",
- uprobe ? "uprobe" : "kprobe",
- libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
- return type;
+ if (uprobe) {
+ pr_warning("failed to determine uprobe perf type %s: %s\n",
+ name,
+ libbpf_strerror_r(type,
+ errmsg, sizeof(errmsg)));
+ } else {
+ /* If we do not have an event_source/../kprobes then we
+ * can try to use kprobe-base event tracing, for details
+ * see ./Documentation/trace/kprobetrace.rst
+ */
+ const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
+ char c[PATH_MAX];
+ int fd, n;
+
+ snprintf(c, sizeof(c), "%s/%s/id", file, name);
+
+ err = use_kprobe_debugfs(name, offset, pid, retprobe);
+ if (err)
+ return err;
+
+ type = PERF_TYPE_TRACEPOINT;
+ fd = open(c, O_RDONLY, 0);
+ if (fd < 0) {
+ pr_warning("failed to open tracepoint %s: %s\n",
+ c, strerror(errno));
+ return -errno;
+ }
+ n = read(fd, c, sizeof(c));
+ close(fd);
+ if (n < 0) {
+ pr_warning("failed to read %s: %s\n",
+ c, strerror(errno));
+ return -errno;
+ }
+ c[n] = '\0';
+ config1 = strtol(c, NULL, 0);
+ attr.size = sizeof(attr);
+ attr.type = type;
+ attr.config = config1;
+ attr.sample_period = 1;
+ attr.wakeup_events = 1;
+ }
+ } else {
+ config1 = ptr_to_u64(name);
+ attr.size = sizeof(attr);
+ attr.type = type;
+ attr.config1 = config1; /* kprobe_func or uprobe_path */
+ attr.config2 = offset; /* kprobe_addr or probe_offset */
}
if (retprobe) {
int bit = uprobe ? determine_uprobe_retprobe_bit()
@@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
}
attr.config |= 1 << bit;
}
- attr.size = sizeof(attr);
- attr.type = type;
- attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
- attr.config2 = offset; /* kprobe_addr or probe_offset */
/* pid filter is meaningful only for uprobes */
pfd = syscall(__NR_perf_event_open, &attr,
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-18 14:54 [bpf-next PATCH] bpf: libbpf, support older style kprobe load John Fastabend
@ 2019-10-18 19:44 ` Daniel Borkmann
2019-10-18 22:01 ` John Fastabend
2019-10-22 2:57 ` Andrii Nakryiko
1 sibling, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2019-10-18 19:44 UTC (permalink / raw)
To: John Fastabend; +Cc: andriin, ast, netdev, bpf
On Fri, Oct 18, 2019 at 07:54:26AM -0700, John Fastabend wrote:
> Following ./Documentation/trace/kprobetrace.rst add support for loading
> kprobes programs on older kernels.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index fcea6988f962..12b3105d112c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> return parse_uint_from_file(file, "config:%d\n");
> }
>
> +static int use_kprobe_debugfs(const char *name,
> + uint64_t offset, int pid, bool retprobe)
offset & pid unused?
> +{
> + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> + int fd = open(file, O_WRONLY | O_APPEND, 0);
> + char buf[PATH_MAX];
> + int err;
> +
> + if (fd < 0) {
> + pr_warning("failed open kprobe_events: %s\n",
> + strerror(errno));
> + return -errno;
> + }
> +
> + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> + retprobe ? 'r' : 'p', name, name);
> +
> + err = write(fd, buf, strlen(buf));
> + close(fd);
> + if (err < 0)
> + return -errno;
> + return 0;
> +}
> +
> static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> uint64_t offset, int pid)
> {
> struct perf_event_attr attr = {};
> char errmsg[STRERR_BUFSIZE];
> + uint64_t config1 = 0;
> int type, pfd, err;
>
> type = uprobe ? determine_uprobe_perf_type()
> : determine_kprobe_perf_type();
> if (type < 0) {
> - pr_warning("failed to determine %s perf type: %s\n",
> - uprobe ? "uprobe" : "kprobe",
> - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> - return type;
> + if (uprobe) {
> + pr_warning("failed to determine uprobe perf type %s: %s\n",
> + name,
> + libbpf_strerror_r(type,
> + errmsg, sizeof(errmsg)));
> + } else {
> + /* If we do not have an event_source/../kprobes then we
> + * can try to use kprobe-base event tracing, for details
> + * see ./Documentation/trace/kprobetrace.rst
> + */
> + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> + char c[PATH_MAX];
> + int fd, n;
> +
> + snprintf(c, sizeof(c), "%s/%s/id", file, name);
> +
> + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> + if (err)
> + return err;
Should we throw a pr_warning() here as well when bailing out?
> + type = PERF_TYPE_TRACEPOINT;
> + fd = open(c, O_RDONLY, 0);
> + if (fd < 0) {
> + pr_warning("failed to open tracepoint %s: %s\n",
> + c, strerror(errno));
> + return -errno;
> + }
> + n = read(fd, c, sizeof(c));
> + close(fd);
> + if (n < 0) {
> + pr_warning("failed to read %s: %s\n",
> + c, strerror(errno));
> + return -errno;
> + }
> + c[n] = '\0';
> + config1 = strtol(c, NULL, 0);
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config = config1;
> + attr.sample_period = 1;
> + attr.wakeup_events = 1;
Is there a reason you set latter two whereas below they are not set,
does it not default to these?
> + }
> + } else {
> + config1 = ptr_to_u64(name);
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config1 = config1; /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> }
> if (retprobe) {
> int bit = uprobe ? determine_uprobe_retprobe_bit()
> @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> }
> attr.config |= 1 << bit;
> }
What happens in case of retprobe, don't you (unwantedly) bail out here
again (even through you've set up the retprobe earlier)?
> - attr.size = sizeof(attr);
> - attr.type = type;
> - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> - attr.config2 = offset; /* kprobe_addr or probe_offset */
>
> /* pid filter is meaningful only for uprobes */
> pfd = syscall(__NR_perf_event_open, &attr,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-18 19:44 ` Daniel Borkmann
@ 2019-10-18 22:01 ` John Fastabend
2019-10-18 22:09 ` Yonghong Song
0 siblings, 1 reply; 12+ messages in thread
From: John Fastabend @ 2019-10-18 22:01 UTC (permalink / raw)
To: Daniel Borkmann, John Fastabend; +Cc: andriin, ast, netdev, bpf
Daniel Borkmann wrote:
> On Fri, Oct 18, 2019 at 07:54:26AM -0700, John Fastabend wrote:
> > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > kprobes programs on older kernels.
> >
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> > tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 73 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index fcea6988f962..12b3105d112c 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> > return parse_uint_from_file(file, "config:%d\n");
> > }
> >
> > +static int use_kprobe_debugfs(const char *name,
> > + uint64_t offset, int pid, bool retprobe)
>
> offset & pid unused?
Well pid should be dropped and I'll add support for offset. I've not
being using offset so missed it here.
>
> > +{
> > + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> > + int fd = open(file, O_WRONLY | O_APPEND, 0);
> > + char buf[PATH_MAX];
> > + int err;
> > +
> > + if (fd < 0) {
> > + pr_warning("failed open kprobe_events: %s\n",
> > + strerror(errno));
> > + return -errno;
> > + }
> > +
> > + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> > + retprobe ? 'r' : 'p', name, name);
> > +
> > + err = write(fd, buf, strlen(buf));
> > + close(fd);
> > + if (err < 0)
> > + return -errno;
> > + return 0;
> > +}
> > +
> > static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > uint64_t offset, int pid)
> > {
> > struct perf_event_attr attr = {};
> > char errmsg[STRERR_BUFSIZE];
> > + uint64_t config1 = 0;
> > int type, pfd, err;
> >
> > type = uprobe ? determine_uprobe_perf_type()
> > : determine_kprobe_perf_type();
> > if (type < 0) {
> > - pr_warning("failed to determine %s perf type: %s\n",
> > - uprobe ? "uprobe" : "kprobe",
> > - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > - return type;
> > + if (uprobe) {
> > + pr_warning("failed to determine uprobe perf type %s: %s\n",
> > + name,
> > + libbpf_strerror_r(type,
> > + errmsg, sizeof(errmsg)));
> > + } else {
> > + /* If we do not have an event_source/../kprobes then we
> > + * can try to use kprobe-base event tracing, for details
> > + * see ./Documentation/trace/kprobetrace.rst
> > + */
> > + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> > + char c[PATH_MAX];
> > + int fd, n;
> > +
> > + snprintf(c, sizeof(c), "%s/%s/id", file, name);
> > +
> > + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> > + if (err)
> > + return err;
>
> Should we throw a pr_warning() here as well when bailing out?
Sure makes sense.
>
> > + type = PERF_TYPE_TRACEPOINT;
> > + fd = open(c, O_RDONLY, 0);
> > + if (fd < 0) {
> > + pr_warning("failed to open tracepoint %s: %s\n",
> > + c, strerror(errno));
> > + return -errno;
> > + }
> > + n = read(fd, c, sizeof(c));
> > + close(fd);
> > + if (n < 0) {
> > + pr_warning("failed to read %s: %s\n",
> > + c, strerror(errno));
> > + return -errno;
> > + }
> > + c[n] = '\0';
> > + config1 = strtol(c, NULL, 0);
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config = config1;
> > + attr.sample_period = 1;
> > + attr.wakeup_events = 1;
>
> Is there a reason you set latter two whereas below they are not set,
> does it not default to these?
We can drop this.
>
> > + }
> > + } else {
> > + config1 = ptr_to_u64(name);
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config1 = config1; /* kprobe_func or uprobe_path */
> > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > }
> > if (retprobe) {
> > int bit = uprobe ? determine_uprobe_retprobe_bit()
> > @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > }
> > attr.config |= 1 << bit;
> > }
>
> What happens in case of retprobe, don't you (unwantedly) bail out here
> again (even through you've set up the retprobe earlier)?
Will fix as well. Looking to see how it passed on my box here but either
way its cryptic so will clean up.
>
> > - attr.size = sizeof(attr);
> > - attr.type = type;
> > - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> > - attr.config2 = offset; /* kprobe_addr or probe_offset */
> >
> > /* pid filter is meaningful only for uprobes */
> > pfd = syscall(__NR_perf_event_open, &attr,
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-18 22:01 ` John Fastabend
@ 2019-10-18 22:09 ` Yonghong Song
0 siblings, 0 replies; 12+ messages in thread
From: Yonghong Song @ 2019-10-18 22:09 UTC (permalink / raw)
To: John Fastabend, Daniel Borkmann; +Cc: Andrii Nakryiko, ast, netdev, bpf
On 10/18/19 3:01 PM, John Fastabend wrote:
> Daniel Borkmann wrote:
>> On Fri, Oct 18, 2019 at 07:54:26AM -0700, John Fastabend wrote:
>>> Following ./Documentation/trace/kprobetrace.rst add support for loading
>>> kprobes programs on older kernels.
>>>
>>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>>> ---
>>> tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 73 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index fcea6988f962..12b3105d112c 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
>>> return parse_uint_from_file(file, "config:%d\n");
>>> }
>>>
>>> +static int use_kprobe_debugfs(const char *name,
>>> + uint64_t offset, int pid, bool retprobe)
>>
>> offset & pid unused?
>
> Well pid should be dropped and I'll add support for offset. I've not
> being using offset so missed it here.
>
>>
>>> +{
>>> + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
>>> + int fd = open(file, O_WRONLY | O_APPEND, 0);
>>> + char buf[PATH_MAX];
>>> + int err;
>>> +
>>> + if (fd < 0) {
>>> + pr_warning("failed open kprobe_events: %s\n",
>>> + strerror(errno));
>>> + return -errno;
>>> + }
>>> +
>>> + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
>>> + retprobe ? 'r' : 'p', name, name);
>>> +
>>> + err = write(fd, buf, strlen(buf));
>>> + close(fd);
>>> + if (err < 0)
>>> + return -errno;
>>> + return 0;
>>> +}
>>> +
>>> static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
>>> uint64_t offset, int pid)
>>> {
>>> struct perf_event_attr attr = {};
>>> char errmsg[STRERR_BUFSIZE];
>>> + uint64_t config1 = 0;
>>> int type, pfd, err;
>>>
>>> type = uprobe ? determine_uprobe_perf_type()
>>> : determine_kprobe_perf_type();
>>> if (type < 0) {
>>> - pr_warning("failed to determine %s perf type: %s\n",
>>> - uprobe ? "uprobe" : "kprobe",
>>> - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
>>> - return type;
>>> + if (uprobe) {
>>> + pr_warning("failed to determine uprobe perf type %s: %s\n",
>>> + name,
>>> + libbpf_strerror_r(type,
>>> + errmsg, sizeof(errmsg)));
>>> + } else {
>>> + /* If we do not have an event_source/../kprobes then we
>>> + * can try to use kprobe-base event tracing, for details
>>> + * see ./Documentation/trace/kprobetrace.rst
>>> + */
>>> + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
>>> + char c[PATH_MAX];
>>> + int fd, n;
>>> +
>>> + snprintf(c, sizeof(c), "%s/%s/id", file, name);
>>> +
>>> + err = use_kprobe_debugfs(name, offset, pid, retprobe);
>>> + if (err)
>>> + return err;
>>
>> Should we throw a pr_warning() here as well when bailing out?
>
> Sure makes sense.
>
>>
>>> + type = PERF_TYPE_TRACEPOINT;
>>> + fd = open(c, O_RDONLY, 0);
>>> + if (fd < 0) {
>>> + pr_warning("failed to open tracepoint %s: %s\n",
>>> + c, strerror(errno));
>>> + return -errno;
>>> + }
>>> + n = read(fd, c, sizeof(c));
>>> + close(fd);
>>> + if (n < 0) {
>>> + pr_warning("failed to read %s: %s\n",
>>> + c, strerror(errno));
>>> + return -errno;
>>> + }
>>> + c[n] = '\0';
>>> + config1 = strtol(c, NULL, 0);
>>> + attr.size = sizeof(attr);
>>> + attr.type = type;
>>> + attr.config = config1;
>>> + attr.sample_period = 1;
>>> + attr.wakeup_events = 1;
>>
>> Is there a reason you set latter two whereas below they are not set,
>> does it not default to these?
>
> We can drop this.
Agreed. attr.sample_period and attr.wakeup_events are used for perf.
Here we run bpf programs and return early, so these two values won't
take effect.
>
>>
>>> + }
>>> + } else {
>>> + config1 = ptr_to_u64(name);
>>> + attr.size = sizeof(attr);
>>> + attr.type = type;
>>> + attr.config1 = config1; /* kprobe_func or uprobe_path */
>>> + attr.config2 = offset; /* kprobe_addr or probe_offset */
>>> }
>>> if (retprobe) {
>>> int bit = uprobe ? determine_uprobe_retprobe_bit()
>>> @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
>>> }
>>> attr.config |= 1 << bit;
>>> }
>>
>> What happens in case of retprobe, don't you (unwantedly) bail out here
>> again (even through you've set up the retprobe earlier)?
>
> Will fix as well. Looking to see how it passed on my box here but either
> way its cryptic so will clean up.
>
>>
>>> - attr.size = sizeof(attr);
>>> - attr.type = type;
>>> - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
>>> - attr.config2 = offset; /* kprobe_addr or probe_offset */
>>>
>>> /* pid filter is meaningful only for uprobes */
>>> pfd = syscall(__NR_perf_event_open, &attr,
>>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-18 14:54 [bpf-next PATCH] bpf: libbpf, support older style kprobe load John Fastabend
2019-10-18 19:44 ` Daniel Borkmann
@ 2019-10-22 2:57 ` Andrii Nakryiko
2019-10-22 5:07 ` John Fastabend
1 sibling, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2019-10-22 2:57 UTC (permalink / raw)
To: John Fastabend
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking, bpf
On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Following ./Documentation/trace/kprobetrace.rst add support for loading
> kprobes programs on older kernels.
>
My main concern with this is that this code is born bit-rotten,
because selftests are never testing the legacy code path. How did you
think about testing this and ensuring that this keeps working going
forward?
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index fcea6988f962..12b3105d112c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> return parse_uint_from_file(file, "config:%d\n");
> }
>
> +static int use_kprobe_debugfs(const char *name,
> + uint64_t offset, int pid, bool retprobe)
> +{
> + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> + int fd = open(file, O_WRONLY | O_APPEND, 0);
> + char buf[PATH_MAX];
> + int err;
> +
> + if (fd < 0) {
> + pr_warning("failed open kprobe_events: %s\n",
> + strerror(errno));
> + return -errno;
errno after pr_warning() call might be clobbered, you need to save it
locally first
> + }
> +
> + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> + retprobe ? 'r' : 'p', name, name);
remember result, check it to detect overflow, and use it in write below?
> +
> + err = write(fd, buf, strlen(buf));
> + close(fd);
> + if (err < 0)
> + return -errno;
> + return 0;
> +}
> +
> static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> uint64_t offset, int pid)
> {
> struct perf_event_attr attr = {};
> char errmsg[STRERR_BUFSIZE];
> + uint64_t config1 = 0;
> int type, pfd, err;
>
> type = uprobe ? determine_uprobe_perf_type()
> : determine_kprobe_perf_type();
> if (type < 0) {
> - pr_warning("failed to determine %s perf type: %s\n",
> - uprobe ? "uprobe" : "kprobe",
> - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> - return type;
> + if (uprobe) {
> + pr_warning("failed to determine uprobe perf type %s: %s\n",
> + name,
> + libbpf_strerror_r(type,
> + errmsg, sizeof(errmsg)));
> + } else {
I think this legacy kprobe setup deserves its own function (maybe even
combine it with use_kprobe_debugfs to do entire attribute setup from A
to Z?)
These 2 levels of nestedness is also unfortunate, how about having two
functions that are filling out perf_event_attr? Something like:
err = determine_perf_probe_attr(...)
if (err)
err = determine_legacy_probe_attr(...)
if (!err)
<bail out>
do perf call here
> + /* If we do not have an event_source/../kprobes then we
> + * can try to use kprobe-base event tracing, for details
> + * see ./Documentation/trace/kprobetrace.rst
> + */
> + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> + char c[PATH_MAX];
what does c stand for?
> + int fd, n;
> +
> + snprintf(c, sizeof(c), "%s/%s/id", file, name);
check result? also, is there a reason to not use
"/sys/kernel/debug/tracing/events/kprobes/" directly in format string?
> +
> + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> + if (err)
> + return err;
> +
> + type = PERF_TYPE_TRACEPOINT;
> + fd = open(c, O_RDONLY, 0);
> + if (fd < 0) {
> + pr_warning("failed to open tracepoint %s: %s\n",
> + c, strerror(errno));
> + return -errno;
> + }
> + n = read(fd, c, sizeof(c));
> + close(fd);
> + if (n < 0) {
> + pr_warning("failed to read %s: %s\n",
> + c, strerror(errno));
It's a bit fishy that you are reading into c and then print out c on
error. Its contents might be corrupted at this point.
And same thing about errno as above.
> + return -errno;
> + }
> + c[n] = '\0';
> + config1 = strtol(c, NULL, 0);
no need for config1 variable, just attr.config = strtol(...)?
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config = config1;
> + attr.sample_period = 1;
> + attr.wakeup_events = 1;
> + }
> + } else {
> + config1 = ptr_to_u64(name);
same, just straight attr.config1 = ... ?
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config1 = config1; /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> }
> if (retprobe) {
> int bit = uprobe ? determine_uprobe_retprobe_bit()
> @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> }
> attr.config |= 1 << bit;
> }
> - attr.size = sizeof(attr);
> - attr.type = type;
> - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> - attr.config2 = offset; /* kprobe_addr or probe_offset */
>
> /* pid filter is meaningful only for uprobes */
> pfd = syscall(__NR_perf_event_open, &attr,
>
What about the detaching? Would closing perf event FD be enough?
Wouldn't we need to clear a probe with -:<event>?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-22 2:57 ` Andrii Nakryiko
@ 2019-10-22 5:07 ` John Fastabend
2019-10-22 7:20 ` Daniel Borkmann
2019-10-22 16:40 ` Andrii Nakryiko
0 siblings, 2 replies; 12+ messages in thread
From: John Fastabend @ 2019-10-22 5:07 UTC (permalink / raw)
To: Andrii Nakryiko, John Fastabend
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking, bpf
Andrii Nakryiko wrote:
> On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > kprobes programs on older kernels.
> >
>
> My main concern with this is that this code is born bit-rotten,
> because selftests are never testing the legacy code path. How did you
> think about testing this and ensuring that this keeps working going
> forward?
>
Well we use it, but I see your point and actually I even broke the retprobe
piece hastily fixing merge conflicts in this patch. When I ran tests on it
I missed running retprobe tests on the set of kernels that would hit that
code.
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> > tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 73 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index fcea6988f962..12b3105d112c 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> > return parse_uint_from_file(file, "config:%d\n");
> > }
> >
> > +static int use_kprobe_debugfs(const char *name,
> > + uint64_t offset, int pid, bool retprobe)
> > +{
> > + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> > + int fd = open(file, O_WRONLY | O_APPEND, 0);
> > + char buf[PATH_MAX];
> > + int err;
> > +
> > + if (fd < 0) {
> > + pr_warning("failed open kprobe_events: %s\n",
> > + strerror(errno));
> > + return -errno;
>
> errno after pr_warning() call might be clobbered, you need to save it
> locally first
Seems so thanks.
>
> > + }
> > +
> > + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> > + retprobe ? 'r' : 'p', name, name);
>
> remember result, check it to detect overflow, and use it in write below?
sure seems more robust. If someone has names longer than PATH_MAX though
it seems a bit much.
>
> > +
> > + err = write(fd, buf, strlen(buf));
> > + close(fd);
> > + if (err < 0)
> > + return -errno;
> > + return 0;
> > +}
> > +
> > static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > uint64_t offset, int pid)
> > {
> > struct perf_event_attr attr = {};
> > char errmsg[STRERR_BUFSIZE];
> > + uint64_t config1 = 0;
> > int type, pfd, err;
> >
> > type = uprobe ? determine_uprobe_perf_type()
> > : determine_kprobe_perf_type();
> > if (type < 0) {
> > - pr_warning("failed to determine %s perf type: %s\n",
> > - uprobe ? "uprobe" : "kprobe",
> > - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > - return type;
> > + if (uprobe) {
> > + pr_warning("failed to determine uprobe perf type %s: %s\n",
> > + name,
> > + libbpf_strerror_r(type,
> > + errmsg, sizeof(errmsg)));
> > + } else {
>
> I think this legacy kprobe setup deserves its own function (maybe even
> combine it with use_kprobe_debugfs to do entire attribute setup from A
> to Z?)
>
> These 2 levels of nestedness is also unfortunate, how about having two
> functions that are filling out perf_event_attr? Something like:
>
> err = determine_perf_probe_attr(...)
> if (err)
> err = determine_legacy_probe_attr(...)
> if (!err)
> <bail out>
> do perf call here
>
Perhaps it makes sense to even uplevel this into the API? Something like
bpf_program__attach_legacy_kprobe() then we could test it easer?
>
> > + /* If we do not have an event_source/../kprobes then we
> > + * can try to use kprobe-base event tracing, for details
> > + * see ./Documentation/trace/kprobetrace.rst
> > + */
> > + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> > + char c[PATH_MAX];
>
> what does c stand for?
Can name it file and push the path into snprintf() below.
>
> > + int fd, n;
> > +
> > + snprintf(c, sizeof(c), "%s/%s/id", file, name);
>
> check result? also, is there a reason to not use
> "/sys/kernel/debug/tracing/events/kprobes/" directly in format string?
>
I believe when I wrote that I just like the it as a const char better.
But no reason if you like it inline better.
> > +
> > + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> > + if (err)
> > + return err;
> > +
> > + type = PERF_TYPE_TRACEPOINT;
> > + fd = open(c, O_RDONLY, 0);
> > + if (fd < 0) {
> > + pr_warning("failed to open tracepoint %s: %s\n",
> > + c, strerror(errno));
> > + return -errno;
> > + }
> > + n = read(fd, c, sizeof(c));
> > + close(fd);
> > + if (n < 0) {
> > + pr_warning("failed to read %s: %s\n",
> > + c, strerror(errno));
>
> It's a bit fishy that you are reading into c and then print out c on
> error. Its contents might be corrupted at this point.
>
> And same thing about errno as above.
Sure just didn't see much point in using yet another buffer. We can
just make the pr_warning general enough that the buffer isn't needed.
>
> > + return -errno;
> > + }
> > + c[n] = '\0';
> > + config1 = strtol(c, NULL, 0);
>
> no need for config1 variable, just attr.config = strtol(...)?
yes, fallout from some code churn. But no reason for it.
>
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config = config1;
> > + attr.sample_period = 1;
> > + attr.wakeup_events = 1;
> > + }
> > + } else {
> > + config1 = ptr_to_u64(name);
>
> same, just straight attr.config1 = ... ?
yes.
>
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config1 = config1; /* kprobe_func or uprobe_path */
> > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > }
> > if (retprobe) {
> > int bit = uprobe ? determine_uprobe_retprobe_bit()
> > @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > }
> > attr.config |= 1 << bit;
> > }
> > - attr.size = sizeof(attr);
> > - attr.type = type;
> > - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> > - attr.config2 = offset; /* kprobe_addr or probe_offset */
> >
> > /* pid filter is meaningful only for uprobes */
> > pfd = syscall(__NR_perf_event_open, &attr,
> >
>
> What about the detaching? Would closing perf event FD be enough?
> Wouldn't we need to clear a probe with -:<event>?
It seems to be enough to close the fd. From an API standpoint might
make sense to have a detach() though if adding an attach().
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-22 5:07 ` John Fastabend
@ 2019-10-22 7:20 ` Daniel Borkmann
2019-10-22 16:41 ` Andrii Nakryiko
2019-10-22 16:40 ` Andrii Nakryiko
1 sibling, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2019-10-22 7:20 UTC (permalink / raw)
To: John Fastabend
Cc: Andrii Nakryiko, Andrii Nakryiko, Alexei Starovoitov, Networking, bpf
On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> Andrii Nakryiko wrote:
> > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > >
> > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > kprobes programs on older kernels.
> >
> > My main concern with this is that this code is born bit-rotten,
> > because selftests are never testing the legacy code path. How did you
> > think about testing this and ensuring that this keeps working going
> > forward?
>
> Well we use it, but I see your point and actually I even broke the retprobe
> piece hastily fixing merge conflicts in this patch. When I ran tests on it
> I missed running retprobe tests on the set of kernels that would hit that
> code.
If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
such, it should be easy to add BPF selftests for that API to address the test
coverage concern. Generally more selftests for exposed libbpf APIs is good to
have anyway.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-22 5:07 ` John Fastabend
2019-10-22 7:20 ` Daniel Borkmann
@ 2019-10-22 16:40 ` Andrii Nakryiko
1 sibling, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2019-10-22 16:40 UTC (permalink / raw)
To: John Fastabend
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking, bpf
On Mon, Oct 21, 2019 at 10:08 PM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > >
> > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > kprobes programs on older kernels.
> > >
> >
> > My main concern with this is that this code is born bit-rotten,
> > because selftests are never testing the legacy code path. How did you
> > think about testing this and ensuring that this keeps working going
> > forward?
> >
>
> Well we use it, but I see your point and actually I even broke the retprobe
> piece hastily fixing merge conflicts in this patch. When I ran tests on it
> I missed running retprobe tests on the set of kernels that would hit that
> code.
>
> > > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > > ---
> > > tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> > > 1 file changed, 73 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index fcea6988f962..12b3105d112c 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> > > return parse_uint_from_file(file, "config:%d\n");
> > > }
> > >
> > > +static int use_kprobe_debugfs(const char *name,
> > > + uint64_t offset, int pid, bool retprobe)
> > > +{
> > > + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> > > + int fd = open(file, O_WRONLY | O_APPEND, 0);
> > > + char buf[PATH_MAX];
> > > + int err;
> > > +
> > > + if (fd < 0) {
> > > + pr_warning("failed open kprobe_events: %s\n",
> > > + strerror(errno));
> > > + return -errno;
> >
> > errno after pr_warning() call might be clobbered, you need to save it
> > locally first
>
> Seems so thanks.
>
> >
> > > + }
> > > +
> > > + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> > > + retprobe ? 'r' : 'p', name, name);
> >
> > remember result, check it to detect overflow, and use it in write below?
>
> sure seems more robust. If someone has names longer than PATH_MAX though
> it seems a bit much.
>
> >
> > > +
> > > + err = write(fd, buf, strlen(buf));
> > > + close(fd);
> > > + if (err < 0)
> > > + return -errno;
> > > + return 0;
> > > +}
> > > +
> > > static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > > uint64_t offset, int pid)
> > > {
> > > struct perf_event_attr attr = {};
> > > char errmsg[STRERR_BUFSIZE];
> > > + uint64_t config1 = 0;
> > > int type, pfd, err;
> > >
> > > type = uprobe ? determine_uprobe_perf_type()
> > > : determine_kprobe_perf_type();
> > > if (type < 0) {
> > > - pr_warning("failed to determine %s perf type: %s\n",
> > > - uprobe ? "uprobe" : "kprobe",
> > > - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > > - return type;
> > > + if (uprobe) {
> > > + pr_warning("failed to determine uprobe perf type %s: %s\n",
> > > + name,
> > > + libbpf_strerror_r(type,
> > > + errmsg, sizeof(errmsg)));
> > > + } else {
> >
> > I think this legacy kprobe setup deserves its own function (maybe even
> > combine it with use_kprobe_debugfs to do entire attribute setup from A
> > to Z?)
> >
> > These 2 levels of nestedness is also unfortunate, how about having two
> > functions that are filling out perf_event_attr? Something like:
> >
> > err = determine_perf_probe_attr(...)
> > if (err)
> > err = determine_legacy_probe_attr(...)
> > if (!err)
> > <bail out>
> > do perf call here
> >
>
> Perhaps it makes sense to even uplevel this into the API? Something like
> bpf_program__attach_legacy_kprobe() then we could test it easer?
Having this as a separate API is bad from usability standpoint,
because we force user to either know which kernel versions support new
vs old ways, or have to write a "try this, if fails - try that" logic,
which is ugly. So I think we should hide this from user.
I'm still thinking what's the least intrusive way to cause new way to
fail on modern kernels, so that we can guarantee that legacy code path
is executed. Some way of forcing determine_kprobe_perf_type() to fail
would be easiest.
>
> >
> > > + /* If we do not have an event_source/../kprobes then we
> > > + * can try to use kprobe-base event tracing, for details
> > > + * see ./Documentation/trace/kprobetrace.rst
> > > + */
> > > + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> > > + char c[PATH_MAX];
> >
> > what does c stand for?
>
> Can name it file and push the path into snprintf() below.
sounds good
>
> >
> > > + int fd, n;
> > > +
> > > + snprintf(c, sizeof(c), "%s/%s/id", file, name);
> >
> > check result? also, is there a reason to not use
> > "/sys/kernel/debug/tracing/events/kprobes/" directly in format string?
> >
>
> I believe when I wrote that I just like the it as a const char better.
> But no reason if you like it inline better.
>
> > > +
> > > + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> > > + if (err)
> > > + return err;
> > > +
> > > + type = PERF_TYPE_TRACEPOINT;
> > > + fd = open(c, O_RDONLY, 0);
> > > + if (fd < 0) {
> > > + pr_warning("failed to open tracepoint %s: %s\n",
> > > + c, strerror(errno));
> > > + return -errno;
> > > + }
> > > + n = read(fd, c, sizeof(c));
> > > + close(fd);
> > > + if (n < 0) {
> > > + pr_warning("failed to read %s: %s\n",
> > > + c, strerror(errno));
> >
> > It's a bit fishy that you are reading into c and then print out c on
> > error. Its contents might be corrupted at this point.
> >
> > And same thing about errno as above.
>
> Sure just didn't see much point in using yet another buffer. We can
> just make the pr_warning general enough that the buffer isn't needed.
just use parse_uint_from_file(), as long as we don't expect to read
negative numbers, it will do what you want (and that's what we already
use for new-style kprobe/uprobe).
>
> >
> > > + return -errno;
> > > + }
> > > + c[n] = '\0';
> > > + config1 = strtol(c, NULL, 0);
> >
> > no need for config1 variable, just attr.config = strtol(...)?
>
> yes, fallout from some code churn. But no reason for it.
>
> >
> > > + attr.size = sizeof(attr);
> > > + attr.type = type;
> > > + attr.config = config1;
> > > + attr.sample_period = 1;
> > > + attr.wakeup_events = 1;
> > > + }
> > > + } else {
> > > + config1 = ptr_to_u64(name);
> >
> > same, just straight attr.config1 = ... ?
>
> yes.
>
> >
> > > + attr.size = sizeof(attr);
> > > + attr.type = type;
> > > + attr.config1 = config1; /* kprobe_func or uprobe_path */
> > > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > > }
> > > if (retprobe) {
> > > int bit = uprobe ? determine_uprobe_retprobe_bit()
> > > @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > > }
> > > attr.config |= 1 << bit;
> > > }
> > > - attr.size = sizeof(attr);
> > > - attr.type = type;
> > > - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> > > - attr.config2 = offset; /* kprobe_addr or probe_offset */
> > >
> > > /* pid filter is meaningful only for uprobes */
> > > pfd = syscall(__NR_perf_event_open, &attr,
> > >
> >
> > What about the detaching? Would closing perf event FD be enough?
> > Wouldn't we need to clear a probe with -:<event>?
>
> It seems to be enough to close the fd. From an API standpoint might
> make sense to have a detach() though if adding an attach().
we do have detach, it's bpf_link__destroy()
BCC certainly deletes kprobe:
https://github.com/iovisor/bcc/blob/master/src/cc/libbpf.c#L1142
BTW, if we are adding legacy stuff, we should probably also add uprobe
support, no? See BCC for inspiration:
https://github.com/iovisor/bcc/blob/master/src/cc/libbpf.c#L975
One thing that's not clear, and maybe Yonghong can clarify, is why do
we need to enter mount_ns. I vaguely remember this has problems for
multi-threaded applications. This aspect libbpf can't and shouldn't
control, so I wonder if we can avoid mount_ns?..
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-22 7:20 ` Daniel Borkmann
@ 2019-10-22 16:41 ` Andrii Nakryiko
2019-10-24 18:55 ` John Fastabend
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2019-10-22 16:41 UTC (permalink / raw)
To: Daniel Borkmann
Cc: John Fastabend, Andrii Nakryiko, Alexei Starovoitov, Networking, bpf
On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> > Andrii Nakryiko wrote:
> > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > >
> > > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > > kprobes programs on older kernels.
> > >
> > > My main concern with this is that this code is born bit-rotten,
> > > because selftests are never testing the legacy code path. How did you
> > > think about testing this and ensuring that this keeps working going
> > > forward?
> >
> > Well we use it, but I see your point and actually I even broke the retprobe
> > piece hastily fixing merge conflicts in this patch. When I ran tests on it
> > I missed running retprobe tests on the set of kernels that would hit that
> > code.
>
> If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
> such, it should be easy to add BPF selftests for that API to address the test
> coverage concern. Generally more selftests for exposed libbpf APIs is good to
> have anyway.
>
Agree about tests. Disagree about more APIs, especially that the only
difference will be which underlying kernel machinery they are using to
set everything up. We should ideally avoid exposing that to users.
> Cheers,
> Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-22 16:41 ` Andrii Nakryiko
@ 2019-10-24 18:55 ` John Fastabend
2019-10-25 3:39 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: John Fastabend @ 2019-10-24 18:55 UTC (permalink / raw)
To: Andrii Nakryiko, Daniel Borkmann
Cc: John Fastabend, Andrii Nakryiko, Alexei Starovoitov, Networking, bpf
Andrii Nakryiko wrote:
> On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> > > Andrii Nakryiko wrote:
> > > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > > >
> > > > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > > > kprobes programs on older kernels.
> > > >
> > > > My main concern with this is that this code is born bit-rotten,
> > > > because selftests are never testing the legacy code path. How did you
> > > > think about testing this and ensuring that this keeps working going
> > > > forward?
> > >
> > > Well we use it, but I see your point and actually I even broke the retprobe
> > > piece hastily fixing merge conflicts in this patch. When I ran tests on it
> > > I missed running retprobe tests on the set of kernels that would hit that
> > > code.
> >
> > If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
> > such, it should be easy to add BPF selftests for that API to address the test
> > coverage concern. Generally more selftests for exposed libbpf APIs is good to
> > have anyway.
> >
>
> Agree about tests. Disagree about more APIs, especially that the only
> difference will be which underlying kernel machinery they are using to
> set everything up. We should ideally avoid exposing that to users.
Maybe a build flag to build with only the older style supported for testing?
Then we could build, test in selftests at least. Be clear the flag is only
for testing and can not be relied upon.
>
> > Cheers,
> > Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-24 18:55 ` John Fastabend
@ 2019-10-25 3:39 ` Andrii Nakryiko
2019-10-25 15:59 ` John Fastabend
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2019-10-25 3:39 UTC (permalink / raw)
To: John Fastabend
Cc: Daniel Borkmann, Andrii Nakryiko, Alexei Starovoitov, Networking, bpf
On Thu, Oct 24, 2019 at 11:55 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> > > > Andrii Nakryiko wrote:
> > > > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > > > >
> > > > > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > > > > kprobes programs on older kernels.
> > > > >
> > > > > My main concern with this is that this code is born bit-rotten,
> > > > > because selftests are never testing the legacy code path. How did you
> > > > > think about testing this and ensuring that this keeps working going
> > > > > forward?
> > > >
> > > > Well we use it, but I see your point and actually I even broke the retprobe
> > > > piece hastily fixing merge conflicts in this patch. When I ran tests on it
> > > > I missed running retprobe tests on the set of kernels that would hit that
> > > > code.
> > >
> > > If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
> > > such, it should be easy to add BPF selftests for that API to address the test
> > > coverage concern. Generally more selftests for exposed libbpf APIs is good to
> > > have anyway.
> > >
> >
> > Agree about tests. Disagree about more APIs, especially that the only
> > difference will be which underlying kernel machinery they are using to
> > set everything up. We should ideally avoid exposing that to users.
>
> Maybe a build flag to build with only the older style supported for testing?
> Then we could build, test in selftests at least. Be clear the flag is only
> for testing and can not be relied upon.
Build flag will necessitate another "flavor" of test_progs just to
test this. That seems like an overkill.
How about this approach:
$ cat silent-features.c
#include <stdio.h>
int __attribute__((weak)) __bpf_internal__force_legacy_kprobe;
int main() {
if (__bpf_internal__force_legacy_kprobe)
printf("LEGACY MODE!\n");
else
printf("FANCY NEW MODE!\n");
return 0;
}
$ cat silent-features-testing.c
int __bpf_internal__force_legacy_kprobe = 1;
$ cc -g -O2 silent-features.c -o silent-features && ./silent-features
FANCY NEW MODE!
$ cc -g -O2 silent-features.c silent-features-testing.c -o
silent-features && ./silent-features
LEGACY MODE!
This seems like an extensible mechanism without introducing any new
public APIs or knobs, and we can control that in runtime. Some good
naming convention to emphasize this is only for testing and internal
needs, and I think it should be fine.
>
> >
> > > Cheers,
> > > Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load
2019-10-25 3:39 ` Andrii Nakryiko
@ 2019-10-25 15:59 ` John Fastabend
0 siblings, 0 replies; 12+ messages in thread
From: John Fastabend @ 2019-10-25 15:59 UTC (permalink / raw)
To: Andrii Nakryiko, John Fastabend
Cc: Daniel Borkmann, Andrii Nakryiko, Alexei Starovoitov, Networking, bpf
Andrii Nakryiko wrote:
> On Thu, Oct 24, 2019 at 11:55 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Andrii Nakryiko wrote:
> > > On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > >
> > > > On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote:
> > > > > Andrii Nakryiko wrote:
> > > > > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > > > > >
> > > > > > > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > > > > > > kprobes programs on older kernels.
> > > > > >
> > > > > > My main concern with this is that this code is born bit-rotten,
> > > > > > because selftests are never testing the legacy code path. How did you
> > > > > > think about testing this and ensuring that this keeps working going
> > > > > > forward?
> > > > >
> > > > > Well we use it, but I see your point and actually I even broke the retprobe
> > > > > piece hastily fixing merge conflicts in this patch. When I ran tests on it
> > > > > I missed running retprobe tests on the set of kernels that would hit that
> > > > > code.
> > > >
> > > > If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or
> > > > such, it should be easy to add BPF selftests for that API to address the test
> > > > coverage concern. Generally more selftests for exposed libbpf APIs is good to
> > > > have anyway.
> > > >
> > >
> > > Agree about tests. Disagree about more APIs, especially that the only
> > > difference will be which underlying kernel machinery they are using to
> > > set everything up. We should ideally avoid exposing that to users.
> >
> > Maybe a build flag to build with only the older style supported for testing?
> > Then we could build, test in selftests at least. Be clear the flag is only
> > for testing and can not be relied upon.
>
> Build flag will necessitate another "flavor" of test_progs just to
> test this. That seems like an overkill.
>
> How about this approach:
Sure sounds good. I'll do this next week along with the uprobe pieces
as well so we can get both kprobe/uprobe running on older kernels.
>
> $ cat silent-features.c
> #include <stdio.h>
>
> int __attribute__((weak)) __bpf_internal__force_legacy_kprobe;
>
> int main() {
> if (__bpf_internal__force_legacy_kprobe)
> printf("LEGACY MODE!\n");
> else
> printf("FANCY NEW MODE!\n");
> return 0;
> }
> $ cat silent-features-testing.c
> int __bpf_internal__force_legacy_kprobe = 1;
> $ cc -g -O2 silent-features.c -o silent-features && ./silent-features
> FANCY NEW MODE!
> $ cc -g -O2 silent-features.c silent-features-testing.c -o
> silent-features && ./silent-features
> LEGACY MODE!
>
> This seems like an extensible mechanism without introducing any new
> public APIs or knobs, and we can control that in runtime. Some good
> naming convention to emphasize this is only for testing and internal
> needs, and I think it should be fine.
>
> >
> > >
> > > > Cheers,
> > > > Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-25 15:59 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 14:54 [bpf-next PATCH] bpf: libbpf, support older style kprobe load John Fastabend
2019-10-18 19:44 ` Daniel Borkmann
2019-10-18 22:01 ` John Fastabend
2019-10-18 22:09 ` Yonghong Song
2019-10-22 2:57 ` Andrii Nakryiko
2019-10-22 5:07 ` John Fastabend
2019-10-22 7:20 ` Daniel Borkmann
2019-10-22 16:41 ` Andrii Nakryiko
2019-10-24 18:55 ` John Fastabend
2019-10-25 3:39 ` Andrii Nakryiko
2019-10-25 15:59 ` John Fastabend
2019-10-22 16:40 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).