netdev - Re: [PATCH v2 bpf-next 3/7] selftests/bpf: adjust CO-RE reloc tests for new bpf_core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Oct 2019 13:41:55 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 3/7] selftests/bpf: adjust CO-RE reloc tests
 for new bpf_core_read() macro

On Thu, Oct 3, 2019 at 1:29 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Oct 3, 2019 at 1:17 PM Song Liu <liu.song.a23@...il.com> wrote:
> >
> > On Wed, Oct 2, 2019 at 3:01 PM Andrii Nakryiko <andriin@...com> wrote:
> > >
> > > To allow adding a variadic BPF_CORE_READ macro with slightly different
> > > syntax and semantics, define CORE_READ in CO-RE reloc tests, which is
> > > a thin wrapper around low-level bpf_core_read() macro, which in turn is
> > > just a wrapper around bpf_probe_read().
> > >
> > > Acked-by: John Fastabend <john.fastabend@...il.com>
> > > Acked-by: Song Liu <songliubraving@...com>
> > > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > > ---
> > >  tools/testing/selftests/bpf/bpf_helpers.h      |  8 ++++----
> > >  .../bpf/progs/test_core_reloc_arrays.c         | 10 ++++++----
> > >  .../bpf/progs/test_core_reloc_flavors.c        |  8 +++++---
> > >  .../selftests/bpf/progs/test_core_reloc_ints.c | 18 ++++++++++--------
> > >  .../bpf/progs/test_core_reloc_kernel.c         |  6 ++++--
> > >  .../selftests/bpf/progs/test_core_reloc_misc.c |  8 +++++---
> > >  .../selftests/bpf/progs/test_core_reloc_mods.c | 18 ++++++++++--------
> > >  .../bpf/progs/test_core_reloc_nesting.c        |  6 ++++--
> > >  .../bpf/progs/test_core_reloc_primitives.c     | 12 +++++++-----
> > >  .../bpf/progs/test_core_reloc_ptr_as_arr.c     |  4 +++-
> > >  10 files changed, 58 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> > > index 7b75c38238e4..5210cc7d7c5c 100644
> > > --- a/tools/testing/selftests/bpf/bpf_helpers.h
> > > +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> > > @@ -483,7 +483,7 @@ struct pt_regs;
> > >  #endif
> > >
> > >  /*
> > > - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> > > + * bpf_core_read() abstracts away bpf_probe_read() call and captures offset
> > >   * relocation for source address using __builtin_preserve_access_index()
> > >   * built-in, provided by Clang.
> > >   *
> > > @@ -498,8 +498,8 @@ struct pt_regs;
> > >   * actual field offset, based on target kernel BTF type that matches original
> > >   * (local) BTF, used to record relocation.
> > >   */
> > > -#define BPF_CORE_READ(dst, src)                                                \
> > > -       bpf_probe_read((dst), sizeof(*(src)),                           \
> > > -                      __builtin_preserve_access_index(src))
> > > +#define bpf_core_read(dst, sz, src)                                        \
> > > +       bpf_probe_read(dst, sz,                                             \
> > > +                      (const void *)__builtin_preserve_access_index(src))
> > >
> > >  #endif
> > > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> > > index bf67f0fdf743..58efe4944594 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> > > @@ -31,6 +31,8 @@ struct core_reloc_arrays {
> > >         struct core_reloc_arrays_substruct d[1][2];
> > >  };
> > >
> > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
> >
> > We are using sizeof(*dst) now, but I guess sizeof(*src) is better?
> > And it should be sizeof(*(src)).
>
> There is no clear winner and I've debated which one I should go with,
> but I'm leaning towards using destination for the following reason.
> Size of destination doesn't change, it's not relocatable and whatnot,
> so this represents actual amount of storage we can safely read into
> (if the program logic is correct, of course). On the other hand, size
> of source might be different between kernels and we don't support
> relocating it when it's passed into bpf_probe_read() as second arg.
>
> There is at least one valid case where we should use destination size,
> not source size: if we have an array of something (e.g, chars) and we
> want to read only up to first N elements. In this case sizeof(*dst) is
> what you really want: program will pre-allocate exact amount of data
> and we'll do, say, char comm[16]; bpf_core_read(dst,
> task_struct->comm). If task_struct->comm ever increases, this all will
> work: we'll read first 16 characters only.
>
> In almost every other case it doesn't matter whether its dst or src,
> they have to match (i.e., we don't support relocation from int32 to
> int64 right now).

Hmm.. We could also reading multiple items into the same array, no?
Maybe we need another marco that takes size as an third parameter?

Also, for dst, it needs to be sizeof(*(dst)).

Thanks,
Song