| Message ID | 20191002215041.1083058-4-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Move bpf_helpers and add BPF_CORE_READ macros | expand |
On Wed, Oct 2, 2019 at 3:01 PM Andrii Nakryiko <andriin@fb.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@gmail.com> > Acked-by: Song Liu <songliubraving@fb.com> > Signed-off-by: Andrii Nakryiko <andriin@fb.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)). > + > SEC("raw_tracepoint/sys_enter") > int test_core_arrays(void *ctx) > { > @@ -38,16 +40,16 @@ int test_core_arrays(void *ctx) > struct core_reloc_arrays_output *out = (void *)&data.out; > > /* in->a[2] */ > - if (BPF_CORE_READ(&out->a2, &in->a[2])) > + if (CORE_READ(&out->a2, &in->a[2])) > return 1; > /* in->b[1][2][3] */ > - if (BPF_CORE_READ(&out->b123, &in->b[1][2][3])) > + if (CORE_READ(&out->b123, &in->b[1][2][3])) > return 1; > /* in->c[1].c */ > - if (BPF_CORE_READ(&out->c1c, &in->c[1].c)) > + if (CORE_READ(&out->c1c, &in->c[1].c)) > return 1; > /* in->d[0][0].d */ > - if (BPF_CORE_READ(&out->d00d, &in->d[0][0].d)) > + if (CORE_READ(&out->d00d, &in->d[0][0].d)) > return 1; > > return 0; > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > index 9fda73e87972..3348acc7e50b 100644 > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > @@ -39,6 +39,8 @@ struct core_reloc_flavors___weird { > }; > }; > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) > + > SEC("raw_tracepoint/sys_enter") > int test_core_flavors(void *ctx) > { > @@ -48,13 +50,13 @@ int test_core_flavors(void *ctx) > struct core_reloc_flavors *out = (void *)&data.out; > > /* read a using weird layout */ > - if (BPF_CORE_READ(&out->a, &in_weird->a)) > + if (CORE_READ(&out->a, &in_weird->a)) > return 1; > /* read b using reversed layout */ > - if (BPF_CORE_READ(&out->b, &in_rev->b)) > + if (CORE_READ(&out->b, &in_rev->b)) > return 1; > /* read c using original layout */ > - if (BPF_CORE_READ(&out->c, &in_orig->c)) > + if (CORE_READ(&out->c, &in_orig->c)) > return 1; > > return 0; > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > index d99233c8008a..cfe16ede48dd 100644 > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > @@ -23,20 +23,22 @@ struct core_reloc_ints { > int64_t s64_field; > }; > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) ditto. > + > SEC("raw_tracepoint/sys_enter") > int test_core_ints(void *ctx) > { > struct core_reloc_ints *in = (void *)&data.in; > struct core_reloc_ints *out = (void *)&data.out; > > - if (BPF_CORE_READ(&out->u8_field, &in->u8_field) || > - BPF_CORE_READ(&out->s8_field, &in->s8_field) || > - BPF_CORE_READ(&out->u16_field, &in->u16_field) || > - BPF_CORE_READ(&out->s16_field, &in->s16_field) || > - BPF_CORE_READ(&out->u32_field, &in->u32_field) || > - BPF_CORE_READ(&out->s32_field, &in->s32_field) || > - BPF_CORE_READ(&out->u64_field, &in->u64_field) || > - BPF_CORE_READ(&out->s64_field, &in->s64_field)) > + if (CORE_READ(&out->u8_field, &in->u8_field) || > + CORE_READ(&out->s8_field, &in->s8_field) || > + CORE_READ(&out->u16_field, &in->u16_field) || > + CORE_READ(&out->s16_field, &in->s16_field) || > + CORE_READ(&out->u32_field, &in->u32_field) || > + CORE_READ(&out->s32_field, &in->s32_field) || > + CORE_READ(&out->u64_field, &in->u64_field) || > + CORE_READ(&out->s64_field, &in->s64_field)) > return 1; > > return 0; > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > index 37e02aa3f0c8..e5308026cfda 100644 > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > @@ -17,6 +17,8 @@ struct task_struct { > int tgid; > }; > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) ditto again, and more below. Thanks, Song
On Thu, Oct 3, 2019 at 1:17 PM Song Liu <liu.song.a23@gmail.com> wrote: > > On Wed, Oct 2, 2019 at 3:01 PM Andrii Nakryiko <andriin@fb.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@gmail.com> > > Acked-by: Song Liu <songliubraving@fb.com> > > Signed-off-by: Andrii Nakryiko <andriin@fb.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). > > > + > > SEC("raw_tracepoint/sys_enter") > > int test_core_arrays(void *ctx) > > { > > @@ -38,16 +40,16 @@ int test_core_arrays(void *ctx) > > struct core_reloc_arrays_output *out = (void *)&data.out; > > > > /* in->a[2] */ > > - if (BPF_CORE_READ(&out->a2, &in->a[2])) > > + if (CORE_READ(&out->a2, &in->a[2])) > > return 1; > > /* in->b[1][2][3] */ > > - if (BPF_CORE_READ(&out->b123, &in->b[1][2][3])) > > + if (CORE_READ(&out->b123, &in->b[1][2][3])) > > return 1; > > /* in->c[1].c */ > > - if (BPF_CORE_READ(&out->c1c, &in->c[1].c)) > > + if (CORE_READ(&out->c1c, &in->c[1].c)) > > return 1; > > /* in->d[0][0].d */ > > - if (BPF_CORE_READ(&out->d00d, &in->d[0][0].d)) > > + if (CORE_READ(&out->d00d, &in->d[0][0].d)) > > return 1; > > > > return 0; > > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > > index 9fda73e87972..3348acc7e50b 100644 > > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c > > @@ -39,6 +39,8 @@ struct core_reloc_flavors___weird { > > }; > > }; > > > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) > > + > > SEC("raw_tracepoint/sys_enter") > > int test_core_flavors(void *ctx) > > { > > @@ -48,13 +50,13 @@ int test_core_flavors(void *ctx) > > struct core_reloc_flavors *out = (void *)&data.out; > > > > /* read a using weird layout */ > > - if (BPF_CORE_READ(&out->a, &in_weird->a)) > > + if (CORE_READ(&out->a, &in_weird->a)) > > return 1; > > /* read b using reversed layout */ > > - if (BPF_CORE_READ(&out->b, &in_rev->b)) > > + if (CORE_READ(&out->b, &in_rev->b)) > > return 1; > > /* read c using original layout */ > > - if (BPF_CORE_READ(&out->c, &in_orig->c)) > > + if (CORE_READ(&out->c, &in_orig->c)) > > return 1; > > > > return 0; > > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > > index d99233c8008a..cfe16ede48dd 100644 > > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c > > @@ -23,20 +23,22 @@ struct core_reloc_ints { > > int64_t s64_field; > > }; > > > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) > > ditto. > > > + > > SEC("raw_tracepoint/sys_enter") > > int test_core_ints(void *ctx) > > { > > struct core_reloc_ints *in = (void *)&data.in; > > struct core_reloc_ints *out = (void *)&data.out; > > > > - if (BPF_CORE_READ(&out->u8_field, &in->u8_field) || > > - BPF_CORE_READ(&out->s8_field, &in->s8_field) || > > - BPF_CORE_READ(&out->u16_field, &in->u16_field) || > > - BPF_CORE_READ(&out->s16_field, &in->s16_field) || > > - BPF_CORE_READ(&out->u32_field, &in->u32_field) || > > - BPF_CORE_READ(&out->s32_field, &in->s32_field) || > > - BPF_CORE_READ(&out->u64_field, &in->u64_field) || > > - BPF_CORE_READ(&out->s64_field, &in->s64_field)) > > + if (CORE_READ(&out->u8_field, &in->u8_field) || > > + CORE_READ(&out->s8_field, &in->s8_field) || > > + CORE_READ(&out->u16_field, &in->u16_field) || > > + CORE_READ(&out->s16_field, &in->s16_field) || > > + CORE_READ(&out->u32_field, &in->u32_field) || > > + CORE_READ(&out->s32_field, &in->s32_field) || > > + CORE_READ(&out->u64_field, &in->u64_field) || > > + CORE_READ(&out->s64_field, &in->s64_field)) > > return 1; > > > > return 0; > > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > index 37e02aa3f0c8..e5308026cfda 100644 > > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c > > @@ -17,6 +17,8 @@ struct task_struct { > > int tgid; > > }; > > > > +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) > ditto again, and more below. > > Thanks, > Song
On Thu, Oct 3, 2019 at 1:29 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 3, 2019 at 1:17 PM Song Liu <liu.song.a23@gmail.com> wrote: > > > > On Wed, Oct 2, 2019 at 3:01 PM Andrii Nakryiko <andriin@fb.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@gmail.com> > > > Acked-by: Song Liu <songliubraving@fb.com> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.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
On Thu, Oct 3, 2019 at 1:42 PM Song Liu <liu.song.a23@gmail.com> wrote: > > On Thu, Oct 3, 2019 at 1:29 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Oct 3, 2019 at 1:17 PM Song Liu <liu.song.a23@gmail.com> wrote: > > > > > > On Wed, Oct 2, 2019 at 3:01 PM Andrii Nakryiko <andriin@fb.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@gmail.com> > > > > Acked-by: Song Liu <songliubraving@fb.com> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.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? Yeah, absolutely, there are cases in which BPF_CORE_READ won't work, unfortunately. That's why it was an internal debate, because there is no perfect answer :) > Maybe we need another marco that takes size as an third parameter? So my thinking for cases that are not compatible with BPF_CORE_READ intended use cases was that users will just do bpf_core_read(), which accepts same params as bpf_probe_read(), so they can do whatever they need to do. > > Also, for dst, it needs to be sizeof(*(dst)). You mean extra () around dst? Sure, will add. > > Thanks, > Song
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) + SEC("raw_tracepoint/sys_enter") int test_core_arrays(void *ctx) { @@ -38,16 +40,16 @@ int test_core_arrays(void *ctx) struct core_reloc_arrays_output *out = (void *)&data.out; /* in->a[2] */ - if (BPF_CORE_READ(&out->a2, &in->a[2])) + if (CORE_READ(&out->a2, &in->a[2])) return 1; /* in->b[1][2][3] */ - if (BPF_CORE_READ(&out->b123, &in->b[1][2][3])) + if (CORE_READ(&out->b123, &in->b[1][2][3])) return 1; /* in->c[1].c */ - if (BPF_CORE_READ(&out->c1c, &in->c[1].c)) + if (CORE_READ(&out->c1c, &in->c[1].c)) return 1; /* in->d[0][0].d */ - if (BPF_CORE_READ(&out->d00d, &in->d[0][0].d)) + if (CORE_READ(&out->d00d, &in->d[0][0].d)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c index 9fda73e87972..3348acc7e50b 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c @@ -39,6 +39,8 @@ struct core_reloc_flavors___weird { }; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_flavors(void *ctx) { @@ -48,13 +50,13 @@ int test_core_flavors(void *ctx) struct core_reloc_flavors *out = (void *)&data.out; /* read a using weird layout */ - if (BPF_CORE_READ(&out->a, &in_weird->a)) + if (CORE_READ(&out->a, &in_weird->a)) return 1; /* read b using reversed layout */ - if (BPF_CORE_READ(&out->b, &in_rev->b)) + if (CORE_READ(&out->b, &in_rev->b)) return 1; /* read c using original layout */ - if (BPF_CORE_READ(&out->c, &in_orig->c)) + if (CORE_READ(&out->c, &in_orig->c)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c index d99233c8008a..cfe16ede48dd 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c @@ -23,20 +23,22 @@ struct core_reloc_ints { int64_t s64_field; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_ints(void *ctx) { struct core_reloc_ints *in = (void *)&data.in; struct core_reloc_ints *out = (void *)&data.out; - if (BPF_CORE_READ(&out->u8_field, &in->u8_field) || - BPF_CORE_READ(&out->s8_field, &in->s8_field) || - BPF_CORE_READ(&out->u16_field, &in->u16_field) || - BPF_CORE_READ(&out->s16_field, &in->s16_field) || - BPF_CORE_READ(&out->u32_field, &in->u32_field) || - BPF_CORE_READ(&out->s32_field, &in->s32_field) || - BPF_CORE_READ(&out->u64_field, &in->u64_field) || - BPF_CORE_READ(&out->s64_field, &in->s64_field)) + if (CORE_READ(&out->u8_field, &in->u8_field) || + CORE_READ(&out->s8_field, &in->s8_field) || + CORE_READ(&out->u16_field, &in->u16_field) || + CORE_READ(&out->s16_field, &in->s16_field) || + CORE_READ(&out->u32_field, &in->u32_field) || + CORE_READ(&out->s32_field, &in->s32_field) || + CORE_READ(&out->u64_field, &in->u64_field) || + CORE_READ(&out->s64_field, &in->s64_field)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c index 37e02aa3f0c8..e5308026cfda 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c @@ -17,6 +17,8 @@ struct task_struct { int tgid; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_kernel(void *ctx) { @@ -24,8 +26,8 @@ int test_core_kernel(void *ctx) uint64_t pid_tgid = bpf_get_current_pid_tgid(); int pid, tgid; - if (BPF_CORE_READ(&pid, &task->pid) || - BPF_CORE_READ(&tgid, &task->tgid)) + if (CORE_READ(&pid, &task->pid) || + CORE_READ(&tgid, &task->tgid)) return 1; /* validate pid + tgid matches */ diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c b/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c index c59984bd3e23..40c4638ab5cc 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c @@ -32,6 +32,8 @@ struct core_reloc_misc_extensible { int b; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_misc(void *ctx) { @@ -41,15 +43,15 @@ int test_core_misc(void *ctx) struct core_reloc_misc_output *out = (void *)&data.out; /* record two different relocations with the same accessor string */ - if (BPF_CORE_READ(&out->a, &in_a->a1) || /* accessor: 0:0 */ - BPF_CORE_READ(&out->b, &in_b->b1)) /* accessor: 0:0 */ + if (CORE_READ(&out->a, &in_a->a1) || /* accessor: 0:0 */ + CORE_READ(&out->b, &in_b->b1)) /* accessor: 0:0 */ return 1; /* Validate relocations capture array-only accesses for structs with * fixed header, but with potentially extendable tail. This will read * first 4 bytes of 2nd element of in_ext array of potentially * variably sized struct core_reloc_misc_extensible. */ - if (BPF_CORE_READ(&out->c, &in_ext[2])) /* accessor: 2 */ + if (CORE_READ(&out->c, &in_ext[2])) /* accessor: 2 */ return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c b/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c index f98b942c062b..99fc9ee21895 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c @@ -41,20 +41,22 @@ struct core_reloc_mods { core_reloc_mods_substruct_t h; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_mods(void *ctx) { struct core_reloc_mods *in = (void *)&data.in; struct core_reloc_mods_output *out = (void *)&data.out; - if (BPF_CORE_READ(&out->a, &in->a) || - BPF_CORE_READ(&out->b, &in->b) || - BPF_CORE_READ(&out->c, &in->c) || - BPF_CORE_READ(&out->d, &in->d) || - BPF_CORE_READ(&out->e, &in->e[2]) || - BPF_CORE_READ(&out->f, &in->f[1]) || - BPF_CORE_READ(&out->g, &in->g.x) || - BPF_CORE_READ(&out->h, &in->h.y)) + if (CORE_READ(&out->a, &in->a) || + CORE_READ(&out->b, &in->b) || + CORE_READ(&out->c, &in->c) || + CORE_READ(&out->d, &in->d) || + CORE_READ(&out->e, &in->e[2]) || + CORE_READ(&out->f, &in->f[1]) || + CORE_READ(&out->g, &in->g.x) || + CORE_READ(&out->h, &in->h.y)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c b/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c index 3ca30cec2b39..c84fff3bdd72 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c @@ -30,15 +30,17 @@ struct core_reloc_nesting { } b; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_nesting(void *ctx) { struct core_reloc_nesting *in = (void *)&data.in; struct core_reloc_nesting *out = (void *)&data.out; - if (BPF_CORE_READ(&out->a.a.a, &in->a.a.a)) + if (CORE_READ(&out->a.a.a, &in->a.a.a)) return 1; - if (BPF_CORE_READ(&out->b.b.b, &in->b.b.b)) + if (CORE_READ(&out->b.b.b, &in->b.b.b)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c b/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c index add52f23ab35..4038e9907162 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c @@ -25,17 +25,19 @@ struct core_reloc_primitives { int (*f)(const char *); }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_primitives(void *ctx) { struct core_reloc_primitives *in = (void *)&data.in; struct core_reloc_primitives *out = (void *)&data.out; - if (BPF_CORE_READ(&out->a, &in->a) || - BPF_CORE_READ(&out->b, &in->b) || - BPF_CORE_READ(&out->c, &in->c) || - BPF_CORE_READ(&out->d, &in->d) || - BPF_CORE_READ(&out->f, &in->f)) + if (CORE_READ(&out->a, &in->a) || + CORE_READ(&out->b, &in->b) || + CORE_READ(&out->c, &in->c) || + CORE_READ(&out->d, &in->d) || + CORE_READ(&out->f, &in->f)) return 1; return 0; diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c index 526b7ddc7ea1..414d44620258 100644 --- a/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c @@ -16,13 +16,15 @@ struct core_reloc_ptr_as_arr { int a; }; +#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src) + SEC("raw_tracepoint/sys_enter") int test_core_ptr_as_arr(void *ctx) { struct core_reloc_ptr_as_arr *in = (void *)&data.in; struct core_reloc_ptr_as_arr *out = (void *)&data.out; - if (BPF_CORE_READ(&out->a, &in[2].a)) + if (CORE_READ(&out->a, &in[2].a)) return 1; return 0;