On Thu, Oct 1, 2020 at 1:09 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Sep 15, 2020 at 12:26 AM Luka Oreskovic
> <luka.oreskovic@xxxxxxxxxx> wrote:
> >
> > On Mon, Sep 14, 2020 at 7:49 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 14, 2020 at 12:55 AM Luka Oreskovic
> > > <luka.oreskovic@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Sep 11, 2020 at 8:14 PM Andrii Nakryiko
> > > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Sep 11, 2020 at 9:45 AM Luka Oreskovic
> > > > > <luka.oreskovic@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Greetings everyone,
> > > > > >
> > > > > > I have been testing various BPF programs on the ARM32 architecture and
> > > > > > have encountered a strange error.
> > > > > >
> > > > > > When trying to run a simple program that prints out the arguments of
> > > > > > the open syscall,
> > > > > > I found some strange behaviour with the pointer offsets when accessing
> > > > > > the arguments:
> > > > > > The output of llvm-objdump differed from the verifier error dump log.
> > > > > > Notice the differences in lines 0 and 1. Why is the bytecode being
> > > > > > altered at runtime?
> > > > > >
> > > > > > I attached the program, the llvm-objdump result and the verifier dump below.
> > > > > >
> > > > > > Best wishes,
> > > > > > Luka Oreskovic
> > > > > >
> > > > > > BPF program
> > > > > > --------------------------------------------
> > > > > > #include "vmlinux.h"
> > > > > > #include <bpf/bpf_helpers.h>
> > > > > >
> > > > > > SEC("tracepoint/syscalls/sys_enter_open")
> > > > > > int tracepoint__syscalls__sys_enter_open(struct trace_event_raw_sys_enter* ctx)
> > > > > > {
> > > > > > const char *arg1 = (const char *)ctx->args[0];
> > > > > > int arg2 = (int)ctx->args[1];
>
> Luka, can you apply the changes below to bpf_core_read.h header and
> read these args using BPF_CORE_READ() macro:
>
> const char *arg1 = (const char *)BPF_CORE_READ(ctx, args[0]);
> int arg2 = BPF_CORE_READ(ctx, args[1]);
>
> I'm curious if that will work (unfortunately I don't have a complete
> enough setup to test this).
>
> The patch is as follows (with broken tab<->space conversion, so please
> make changes by hand):
>
> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
> index bbcefb3ff5a5..fee6328d36c0 100644
> --- a/tools/lib/bpf/bpf_core_read.h
> +++ b/tools/lib/bpf/bpf_core_read.h
> @@ -261,14 +261,16 @@ enum bpf_enum_value_kind {
> #define ___type(...) typeof(___arrow(__VA_ARGS__))
>
> #define ___read(read_fn, dst, src_type, src, accessor) \
> - read_fn((void *)(dst), sizeof(*(dst)), &((src_type)(src))->accessor)
> + read_fn((void *)(dst), \
> + bpf_core_field_size(((src_type)(src))->accessor), \
> + &((src_type)(src))->accessor)
>
> /* "recursively" read a sequence of inner pointers using local __t var */
> #define ___rd_first(src, a) ___read(bpf_core_read, &__t, ___type(src), src, a);
> #define ___rd_last(...)
> \
> ___read(bpf_core_read, &__t, \
> ___type(___nolast(__VA_ARGS__)), __t, ___last(__VA_ARGS__));
> -#define ___rd_p1(...) const void *__t; ___rd_first(__VA_ARGS__)
> +#define ___rd_p1(...) const void *__t = (void *)0; ___rd_first(__VA_ARGS__)
> #define ___rd_p2(...) ___rd_p1(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
> #define ___rd_p3(...) ___rd_p2(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
> #define ___rd_p4(...) ___rd_p3(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
>
>
>
> BTW, this approach should work for reading pointers as well, it would
> be nice if you can test that as well. E.g., something like the
> following:
>
> struct task_struct *t = (void *)bpf_get_current_task();
> int ppid = BPF_CORE_READ(t, group_leader, tgid);
>
> If you try it without the patch above, it should either read garbage
> or zero, but not a valid parent PID (please verify that as well).
>
> I really appreciate your help with testing, thanks!
>
>
> > > > > >
> > > > > > bpf_printk("Open arg 1: %s\n", arg1);
> > > > > > bpf_printk("Open arg 2: %d\n", arg2);
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > char LICENSE[] SEC("license") = "GPL";
> > > > > >
> > > > > >
>
> [...]
>
> >
> > Best wishes,
> > Luka Oreskovic
Greetings,
I have tested your patch using the BPF_CORE_READ() macro and
everything works great!
As for the pointer read, it prints valid PIDs both with the patch and
without it.
Thank you very much for your help.
