| Message ID | 1554925833-7333-12-git-send-email-jiong.wang@netronome.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: eliminate zero extensions for sub-register writes | expand |
On Wed, 10 Apr 2019 20:50:25 +0100, Jiong Wang wrote: > Enable BPF_F_TEST_RND_HI32 for all existing bpf selftests or other > independent tests could involve quite a few changes to make sure all bpf > prog load places has BPF_F_TEST_RND_HI32 set. > > Given most of the tests are using libbpf, this patch introduces a new > global variable "libbpf_test_mode" into libbpf, once which is set, all bpf > prog load issued through libbpf will have BPF_F_TEST_RND_HI32 set > automatically, this could minimize changes required from testsuite. > > The other way might be introducing new load function like > "bpf_prog_test_load", which will set BPF_F_TEST_RND_HI32. But there are > several prog load APIs, and we need minor changes on some parameters. > > The global variable approach seems to be a proper first step for easy > testsuite porting. > > Signed-off-by: Jiong Wang <jiong.wang@netronome.com> Can we perhaps make it per-object by setting it after bpf_object__open() but before bpf_object__load(), or add it to struct bpf_object_open_attr?
> On 11 Apr 2019, at 04:19, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 10 Apr 2019 20:50:25 +0100, Jiong Wang wrote: >> Enable BPF_F_TEST_RND_HI32 for all existing bpf selftests or other >> independent tests could involve quite a few changes to make sure all bpf >> prog load places has BPF_F_TEST_RND_HI32 set. >> >> Given most of the tests are using libbpf, this patch introduces a new >> global variable "libbpf_test_mode" into libbpf, once which is set, all bpf >> prog load issued through libbpf will have BPF_F_TEST_RND_HI32 set >> automatically, this could minimize changes required from testsuite. >> >> The other way might be introducing new load function like >> "bpf_prog_test_load", which will set BPF_F_TEST_RND_HI32. But there are >> several prog load APIs, and we need minor changes on some parameters. >> >> The global variable approach seems to be a proper first step for easy >> testsuite porting. >> >> Signed-off-by: Jiong Wang <jiong.wang@netronome.com> > > Can we perhaps make it per-object by setting it after > bpf_object__open() but before bpf_object__load(), or add > it to struct bpf_object_open_attr? Not sure I followed the meaning correctly. My read is you mean it would better if there is more accurate and fine control on this feature, especially if prog_flags could become an API parameter during file/obj/prog load? As mentioned in the cover letter, I tried to implement bpf_prog_test_load In parallel with existing bpf_prog_test_run, but then found there are also similar APIs for bpf_object__open, and raw insn load “bpf_load_progams” etc and they may involve extending bpf_prog_load_attr/bpf_load_program_attr etc. I thought the change is a little bit heavy. I could go further if it is thought to be the correct way or there is any other suggestion on nicely passing “prog_flags” to low level bpf syscall in libbpf. Regards, Jiong
> On 11 Apr 2019, at 15:32, Jiong Wang <jiong.wang@netronome.com> wrote: > > >> On 11 Apr 2019, at 04:19, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: >> >> On Wed, 10 Apr 2019 20:50:25 +0100, Jiong Wang wrote: >>> Enable BPF_F_TEST_RND_HI32 for all existing bpf selftests or other >>> independent tests could involve quite a few changes to make sure all bpf >>> prog load places has BPF_F_TEST_RND_HI32 set. >>> >>> Given most of the tests are using libbpf, this patch introduces a new >>> global variable "libbpf_test_mode" into libbpf, once which is set, all bpf >>> prog load issued through libbpf will have BPF_F_TEST_RND_HI32 set >>> automatically, this could minimize changes required from testsuite. >>> >>> The other way might be introducing new load function like >>> "bpf_prog_test_load", which will set BPF_F_TEST_RND_HI32. But there are >>> several prog load APIs, and we need minor changes on some parameters. >>> >>> The global variable approach seems to be a proper first step for easy >>> testsuite porting. >>> >>> Signed-off-by: Jiong Wang <jiong.wang@netronome.com> >> >> Can we perhaps make it per-object by setting it after >> bpf_object__open() but before bpf_object__load(), or add >> it to struct bpf_object_open_attr? > > Not sure I followed the meaning correctly. My read is you mean it would > better if there is more accurate and fine control on this feature, especially > if prog_flags could become an API parameter during file/obj/prog load? > > As mentioned in the cover letter, I tried to implement bpf_prog_test_load > In parallel with existing bpf_prog_test_run, but then found there are > also similar APIs for bpf_object__open, and raw insn load “bpf_load_progams” > etc and they may involve extending bpf_prog_load_attr/bpf_load_program_attr > etc. I thought the change is a little bit heavy. I could go further if it > is thought to be the correct way or there is any other suggestion on nicely > passing “prog_flags” to low level bpf syscall in libbpf. > Would really appreciate more feedbacks on how to do the libbpf change for the next patch set re-spin. Whether this global variable approach is acceptable or we must find another way which enable high 32-bit randomization for all tests under test_verifier/test_progs with reasonable testsuite and libbpf change. I feel the global variable approach taken in this patch has done its job with minimum change, that is to offer a interface to enable high 32-bit randomization for all tests under test_verifier/test_progs to deliver equal stressfull tests on x86_64 platform and to expose bugs in the optimization, now given there is no regression, we could have confidence on the correctness of the optimization. If we want a more clean change in libbpf, it is more about how to offer clean API to pass "prog_flags" when load file/obj/raw_insn, it looks to me could be a separate patch set. However, for this set, we also need to make sure high 32-bit randomization *always enabled* for all test_verifier + test_progs so the new opt is tested in daily bpf selftest. Regards, Jiong
> If we want a more clean change in libbpf, it is more about how to offer clean API to > pass "prog_flags" when load file/obj/raw_insn, I followed this path a little bit further: - naturally allow specify prog_flags through *_attr structure. - set high 32-bit randomization prog_flags through macro direct. Now, the global variable in libbpf removed, all tests in selftests are still enabled with hi32 randomization and the code changes on testsuites also is little. Not sure if the idea is good, changes included in v3. Regards, Jiong
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index dababce..0795a85 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -254,6 +254,8 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, if (load_attr->name) memcpy(attr.prog_name, load_attr->name, min(strlen(load_attr->name), BPF_OBJ_NAME_LEN - 1)); + if (libbpf_test_mode) + attr.prog_flags |= BPF_F_TEST_RND_HI32; fd = sys_bpf_prog_load(&attr, sizeof(attr)); if (fd >= 0) @@ -350,6 +352,8 @@ int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns, log_buf[0] = 0; attr.kern_version = kern_version; attr.prog_flags = prog_flags; + if (libbpf_test_mode) + attr.prog_flags |= BPF_F_TEST_RND_HI32; return sys_bpf_prog_load(&attr, sizeof(attr)); } diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index e2a457e..606643f 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -45,6 +45,8 @@ #include "str_error.h" #include "libbpf_util.h" +bool libbpf_test_mode; + #ifndef EM_BPF #define EM_BPF 247 #endif diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index c5ff005..b40de66 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -447,6 +447,8 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); LIBBPF_API void bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); +LIBBPF_API extern bool libbpf_test_mode; + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 6730017..5854e0b 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -156,6 +156,7 @@ LIBBPF_0.0.2 { bpf_program__get_prog_info_linear; bpf_program__bpil_addr_to_offs; bpf_program__bpil_offs_to_addr; + libbpf_test_mode; } LIBBPF_0.0.1; LIBBPF_0.0.3 {
Enable BPF_F_TEST_RND_HI32 for all existing bpf selftests or other independent tests could involve quite a few changes to make sure all bpf prog load places has BPF_F_TEST_RND_HI32 set. Given most of the tests are using libbpf, this patch introduces a new global variable "libbpf_test_mode" into libbpf, once which is set, all bpf prog load issued through libbpf will have BPF_F_TEST_RND_HI32 set automatically, this could minimize changes required from testsuite. The other way might be introducing new load function like "bpf_prog_test_load", which will set BPF_F_TEST_RND_HI32. But there are several prog load APIs, and we need minor changes on some parameters. The global variable approach seems to be a proper first step for easy testsuite porting. Signed-off-by: Jiong Wang <jiong.wang@netronome.com> --- tools/lib/bpf/bpf.c | 4 ++++ tools/lib/bpf/libbpf.c | 2 ++ tools/lib/bpf/libbpf.h | 2 ++ tools/lib/bpf/libbpf.map | 1 + 4 files changed, 9 insertions(+)