[PATCH] samples/bpf: Support both enter and exit kprobes in helper

[PATCH] samples/bpf: Support both enter and exit kprobes in helper

[Lior Ribak]

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(-)

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",
-- 
2.25.1

[PATCH] samples/bpf: Support both enter and exit kprobes in helper

[Lior Ribak]

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(-)

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",
-- 
2.25.1

Re: [PATCH] samples/bpf: Support both enter and exit kprobes in helper

[Yonghong Song]

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",
> 

Re: [PATCH] samples/bpf: Support both enter and exit kprobes in helper

[Lior Ribak]

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",
>> 

Re: [PATCH] samples/bpf: Support both enter and exit kprobes in helper

[Andrii Nakryiko]

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(-)
> >>

[...]