samples: bpf: fix: error handling regarding kprobe_events

Currently, kprobe_events failure won't be handled properly.
Due to calling system() indirectly to write to kprobe_events,
it can't be identified whether an error is derived from kprobe or system.

    // buf = "echo '%c:%s %s' >> /s/k/d/t/kprobe_events"
    err = system(buf);
    if (err < 0) {
        printf("failed to create kprobe ..");
        return -1;
    }

For example, running ./tracex7 sample in ext4 partition,
"echo p:open_ctree open_ctree >> /s/k/d/t/kprobe_events"
gets 256 error code system() failure.
=> The error comes from kprobe, but it's not handled correctly.

According to man of system(3), it's return value
just passes the termination status of the child shell
rather than treating the error as -1. (don't care success)

Which means, currently it's not working as desired.
(According to the upper code snippet)

    ex) running ./tracex7 with ext4 env.
    # Current Output
    sh: echo: I/O error
    failed to open event open_ctree

    # Desired Output
    failed to create kprobe 'open_ctree' error 'No such file or directory'

The problem is, error can't be verified whether from child ps or system.

But using write() directly can verify the command failure,
and it will treat all error as -1.

So I suggest using write() directly to 'kprobe_events'
rather than calling system().

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/bpf_load.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

On 11/21/2018 05:07 PM, Daniel T. Lee wrote:
> Currently, kprobe_events failure won't be handled properly.
> Due to calling system() indirectly to write to kprobe_events,
> it can't be identified whether an error is derived from kprobe or system.
> 
>     // buf = "echo '%c:%s %s' >> /s/k/d/t/kprobe_events"
>     err = system(buf);
>     if (err < 0) {
>         printf("failed to create kprobe ..");
>         return -1;
>     }
> 
> For example, running ./tracex7 sample in ext4 partition,
> "echo p:open_ctree open_ctree >> /s/k/d/t/kprobe_events"
> gets 256 error code system() failure.
> => The error comes from kprobe, but it's not handled correctly.
> 
> According to man of system(3), it's return value
> just passes the termination status of the child shell
> rather than treating the error as -1. (don't care success)
> 
> Which means, currently it's not working as desired.
> (According to the upper code snippet)
> 
>     ex) running ./tracex7 with ext4 env.
>     # Current Output
>     sh: echo: I/O error
>     failed to open event open_ctree
> 
>     # Desired Output
>     failed to create kprobe 'open_ctree' error 'No such file or directory'
> 
> The problem is, error can't be verified whether from child ps or system.
> 
> But using write() directly can verify the command failure,
> and it will treat all error as -1.
> 
> So I suggest using write() directly to 'kprobe_events'
> rather than calling system().
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>

Looks reasonable to me, thanks for the patch! One tiny nit below:

> ---
>  samples/bpf/bpf_load.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> index e6d7e0fe155b..3e34d733f5f8 100644
> --- a/samples/bpf/bpf_load.c
> +++ b/samples/bpf/bpf_load.c
> @@ -54,6 +54,23 @@ static int populate_prog_array(const char *event, int prog_fd)
>  	return 0;
>  }
>  
> +static int write_kprobe_events(const char *val)
> +{
> +	int ret, flags;
> +
> +	if ((val != NULL) && (val[0] == '\0'))
> +		flags = O_WRONLY | O_TRUNC;
> +	else
> +		flags = O_WRONLY | O_APPEND;
> +
> +	int fd = open("/sys/kernel/debug/tracing/kprobe_events", flags);

Nit: could you move fd declaration to above where you have ret and
flags instead of mixing with the code in order to match kernel style?

> +	ret = write(fd, val, strlen(val));
> +	close(fd);
> +
> +	return ret;
> +}
> +
>  static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>  {
>  	bool is_socket = strncmp(event, "socket", 6) == 0;
> @@ -165,10 +182,9 @@ 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),
> -				 "echo '%c:__x64_%s __x64_%s' >> /sys/kernel/debug/tracing/kprobe_events",
> -				 is_kprobe ? 'p' : 'r', event, event);
> -			err = system(buf);
> +			snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s",
> +				is_kprobe ? 'p' : 'r', event, event);
> +			err = write_kprobe_events(buf);
>  			if (err >= 0) {
>  				need_normal_check = false;
>  				event_prefix = "__x64_";
> @@ -176,10 +192,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>  		}
>  #endif
>  		if (need_normal_check) {
> -			snprintf(buf, sizeof(buf),
> -				 "echo '%c:%s %s' >> /sys/kernel/debug/tracing/kprobe_events",
> -				 is_kprobe ? 'p' : 'r', event, event);
> -			err = system(buf);
> +			snprintf(buf, sizeof(buf), "%c:%s %s",
> +				is_kprobe ? 'p' : 'r', event, event);
> +			err = write_kprobe_events(buf);
>  			if (err < 0) {
>  				printf("failed to create kprobe '%s' error '%s'\n",
>  				       event, strerror(errno));
> @@ -519,7 +534,7 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
>  		return 1;
>  
>  	/* clear all kprobes */
> -	i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
> +	i = write_kprobe_events("");
>  
>  	/* scan over all elf sections to get license and map info */
>  	for (i = 1; i < ehdr.e_shnum; i++) {
> 

Thanks,
Daniel