| Message ID | 20200815195726.305131-1-liorribak@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | samples/bpf: Support both enter and exit kprobes in helper | expand |
On 8/15/20 12:57 PM, Lior Ribak wrote: > Currently, in bpf_load.c, the function write_kprobe_events sets > the function name to probe as the probe name. > Even though it's valid to set one kprobe on enter and another on exit, > bpf_load.c won't handle it, and will return an error 'File exists'. > > Add a prefix to the event name to indicate if it's on enter or exit, > so both an enter and an exit kprobes can be attached. Only bpf_load.c change and no users here. So do you imply that use use this piece code in your own environment some how? But in that case, not sure whether this patch is necessary or not. The change here is for legacy bpf_load which may go away in the future. I understand this is for debugfs based kprobe_events where current libbpf does not support. But if possible, you should upgrade to use fd-based kprobe which is introduced roughly in/around 4.17 and libbpf has proper support. > > Signed-off-by: Lior Ribak <liorribak@gmail.com> > --- > samples/bpf/bpf_load.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c > index c5ad528f046e..69102358e91a 100644 > --- a/samples/bpf/bpf_load.c > +++ b/samples/bpf/bpf_load.c > @@ -184,18 +184,24 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size) > > #ifdef __x86_64__ > if (strncmp(event, "sys_", 4) == 0) { > - snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s", > - is_kprobe ? 'p' : 'r', event, event); > + if (is_kprobe) > + event_prefix = "__x64_enter_"; > + else > + event_prefix = "__x64_exit_"; > + snprintf(buf, sizeof(buf), "%c:%s%s __x64_%s", > + is_kprobe ? 'p' : 'r', event_prefix, event, event); > err = write_kprobe_events(buf); > - if (err >= 0) { > + if (err >= 0) > need_normal_check = false; > - event_prefix = "__x64_"; > - } > } > #endif > if (need_normal_check) { > - snprintf(buf, sizeof(buf), "%c:%s %s", > - is_kprobe ? 'p' : 'r', event, event); > + if (is_kprobe) > + event_prefix = "enter_"; > + else > + event_prefix = "exit_"; > + snprintf(buf, sizeof(buf), "%c:%s%s %s", > + is_kprobe ? 'p' : 'r', event_prefix, event, event); > err = write_kprobe_events(buf); > if (err < 0) { > printf("failed to create kprobe '%s' error '%s'\n", >
On 8/15/20 11:02 PM, Yonghong Song wrote: >On 8/15/20 12:57 PM, Lior Ribak wrote: >> Currently, in bpf_load.c, the function write_kprobe_events sets >> the function name to probe as the probe name. >> Even though it's valid to set one kprobe on enter and another on exit, >> bpf_load.c won't handle it, and will return an error 'File exists'. >> >> Add a prefix to the event name to indicate if it's on enter or exit, >> so both an enter and an exit kprobes can be attached. > >Only bpf_load.c change and no users here. So do you imply that >use use this piece code in your own environment some how? But in >that case, not sure whether this patch is necessary or not. > >The change here is for legacy bpf_load which may go away in the future. >I understand this is for debugfs based kprobe_events where current >libbpf does not support. But if possible, you should upgrade to use >fd-based kprobe which is introduced roughly in/around 4.17 and libbpf >has proper support. I used the samples and it's toolchain to write my own bpf, so i wrote this patch to save the trouble to the next one who will try to register 2 kprobes on the same function. I still suggest to apply the patch because it solves a bug. As I see it, all the wrappers around ebpf are duplicated everywhere, and fd-based krpobe is already wrapped in the bcc project, so if you suggest to switch and use it, maybe it's better to import some of the code from bcc instead > >> >> Signed-off-by: Lior Ribak <liorribak@gmail.com> >> --- >> samples/bpf/bpf_load.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c >> index c5ad528f046e..69102358e91a 100644 >> --- a/samples/bpf/bpf_load.c >> +++ b/samples/bpf/bpf_load.c >> @@ -184,18 +184,24 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size) >> >> #ifdef __x86_64__ >> if (strncmp(event, "sys_", 4) == 0) { >> - snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s", >> - is_kprobe ? 'p' : 'r', event, event); >> + if (is_kprobe) >> + event_prefix = "__x64_enter_"; >> + else >> + event_prefix = "__x64_exit_"; >> + snprintf(buf, sizeof(buf), "%c:%s%s __x64_%s", >> + is_kprobe ? 'p' : 'r', event_prefix, event, event); >> err = write_kprobe_events(buf); >> - if (err >= 0) { >> + if (err >= 0) >> need_normal_check = false; >> - event_prefix = "__x64_"; >> - } >> } >> #endif >> if (need_normal_check) { >> - snprintf(buf, sizeof(buf), "%c:%s %s", >> - is_kprobe ? 'p' : 'r', event, event); >> + if (is_kprobe) >> + event_prefix = "enter_"; >> + else >> + event_prefix = "exit_"; >> + snprintf(buf, sizeof(buf), "%c:%s%s %s", >> + is_kprobe ? 'p' : 'r', event_prefix, event, event); >> err = write_kprobe_events(buf); >> if (err < 0) { >> printf("failed to create kprobe '%s' error '%s'\n", >>
On Sat, Aug 22, 2020 at 4:30 AM Lior Ribak <liorribak@gmail.com> wrote: > > On 8/15/20 11:02 PM, Yonghong Song wrote: > >On 8/15/20 12:57 PM, Lior Ribak wrote: > >> Currently, in bpf_load.c, the function write_kprobe_events sets > >> the function name to probe as the probe name. > >> Even though it's valid to set one kprobe on enter and another on exit, > >> bpf_load.c won't handle it, and will return an error 'File exists'. > >> > >> Add a prefix to the event name to indicate if it's on enter or exit, > >> so both an enter and an exit kprobes can be attached. > > > > >Only bpf_load.c change and no users here. So do you imply that > >use use this piece code in your own environment some how? But in > >that case, not sure whether this patch is necessary or not. > > > >The change here is for legacy bpf_load which may go away in the future. > >I understand this is for debugfs based kprobe_events where current > >libbpf does not support. But if possible, you should upgrade to use > >fd-based kprobe which is introduced roughly in/around 4.17 and libbpf > >has proper support. > > I used the samples and it's toolchain to write my own bpf, so i wrote this > patch to save the trouble to the next one who will try to register 2 > kprobes on the same function. > I still suggest to apply the patch because it solves a bug. > > As I see it, all the wrappers around ebpf are duplicated everywhere, and > fd-based krpobe is already wrapped in the bcc project, so if you suggest > to switch and use it, maybe it's better to import some of the code from > bcc instead > What Yonghong suggested is to deprecate bpf_load.c completely, including a legacy way to attach kprobe, which will stay connected without proper clean up, if the application crashes. This has been a reason for multiple production problems so far and we've moved away from that, as a community. There is no need to import anything from BCC, libbpf already supports this and much more. samples/bpf unfortunately are a bit outdated (and any help to bring them more in line with modern libbpf usage would be greatly appreciated!), the best place to look at better and more modern examples would be tools/testing/selftests/bpf in Linux repo, or for more realistic examples of building tracing tools, please check [0]. [0] https://github.com/iovisor/bcc/tree/master/libbpf-tools > > > >> > >> Signed-off-by: Lior Ribak <liorribak@gmail.com> > >> --- > >> samples/bpf/bpf_load.c | 20 +++++++++++++------- > >> 1 file changed, 13 insertions(+), 7 deletions(-) > >> [...]
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c index c5ad528f046e..69102358e91a 100644 --- a/samples/bpf/bpf_load.c +++ b/samples/bpf/bpf_load.c @@ -184,18 +184,24 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size) #ifdef __x86_64__ if (strncmp(event, "sys_", 4) == 0) { - snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s", - is_kprobe ? 'p' : 'r', event, event); + if (is_kprobe) + event_prefix = "__x64_enter_"; + else + event_prefix = "__x64_exit_"; + snprintf(buf, sizeof(buf), "%c:%s%s __x64_%s", + is_kprobe ? 'p' : 'r', event_prefix, event, event); err = write_kprobe_events(buf); - if (err >= 0) { + if (err >= 0) need_normal_check = false; - event_prefix = "__x64_"; - } } #endif if (need_normal_check) { - snprintf(buf, sizeof(buf), "%c:%s %s", - is_kprobe ? 'p' : 'r', event, event); + if (is_kprobe) + event_prefix = "enter_"; + else + event_prefix = "exit_"; + snprintf(buf, sizeof(buf), "%c:%s%s %s", + is_kprobe ? 'p' : 'r', event_prefix, event, event); err = write_kprobe_events(buf); if (err < 0) { printf("failed to create kprobe '%s' error '%s'\n",
Currently, in bpf_load.c, the function write_kprobe_events sets the function name to probe as the probe name. Even though it's valid to set one kprobe on enter and another on exit, bpf_load.c won't handle it, and will return an error 'File exists'. Add a prefix to the event name to indicate if it's on enter or exit, so both an enter and an exit kprobes can be attached. Signed-off-by: Lior Ribak <liorribak@gmail.com> --- samples/bpf/bpf_load.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)