On Thu, Oct 1, 2020 at 6:40 AM Luka Oreskovic <luka.oreskovic@xxxxxxxxxx> wrote:
>
> 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!
Ok, glad it worked. It needs to be a bit more work to be applicable in
wide range of situation, but you can use it as a work around for now,
at least.
>
> As for the pointer read, it prints valid PIDs both with the patch and
> without it.
That surprised me initially, but then I realized that 32-bit kernel
will just chop off high 32 bits of BPF's 64-bit register when the
helper is getting passed a pointer, so even if those high 32-bits have
garbage (as should be the case here), it still works magically.
>
> Thank you very much for your help.
