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 <danieltim...@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
