On Thu, Mar 9, 2023 at 1:52 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > > On 3/8/23 04:58, Andrii Nakryiko wrote: > > On Tue, Feb 28, 2023 at 4:27 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > >> > >> I noticed that the verifier behaves incorrectly when attaching to fentry > >> of multiple functions of the same name located in different modules (or > >> in vmlinux). The reason for this is that if the target program is not > >> specified, the verifier will search kallsyms for the trampoline address > >> to attach to. The entire kallsyms is always searched, not respecting the > >> module in which the function to attach to is located. > >> > >> As Yonghong correctly pointed out, there is yet another issue - the > >> trampoline acquires the module reference in register_fentry which means > >> that if the module is unloaded between the place where the address is > >> found in the verifier and register_fentry, it is possible that another > >> module is loaded to the same address in the meantime, which may lead to > >> errors. > >> > >> This patch fixes the above issues by extracting the module name from the > >> BTF of the attachment target (which must be specified) and by doing the > >> search in kallsyms of the correct module. At the same time, the module > >> reference is acquired right after the address is found and only released > >> right before the program itself is unloaded. > >> > > > > is it expected that your newly added test fails on arm64? See [0] > > > > [0] https://github.com/kernel-patches/bpf/actions/runs/4359596129/jobs/7621687719 > > I believe so, the test uses fentry and all fentry/fexit tests are > failing on arm64 with the same error (524) and are disabled in the CI. Then you have to add newly added tests into DENYLIST.aarch64 > > > > >> --- > >> Changes in v9: > >> - two small changes suggested by Jiri Olsa and Jiri's ack > >> > >> Changes in v8: > >> - added module_put to error paths in bpf_check_attach_target after the > >> module reference is acquired > >> > >> Changes in v7: > >> - refactored the module reference manipulation (comments by Jiri Olsa) > >> - cleaned up the test (comments by Andrii Nakryiko) > >> > >> Changes in v6: > >> - storing the module reference inside bpf_prog_aux instead of > >> bpf_trampoline and releasing it when the program is unloaded > >> (suggested by Jiri Olsa) > >> > >> Changes in v5: > >> - fixed acquiring and releasing of module references by trampolines to > >> prevent modules being unloaded between address lookup and trampoline > >> allocation > >> > >> Changes in v4: > >> - reworked module kallsyms lookup approach using existing functions, > >> verifier now calls btf_try_get_module to retrieve the module and > >> find_kallsyms_symbol_value to get the symbol address (suggested by > >> Alexei) > >> - included Jiri Olsa's comments > >> - improved description of the new test and added it as a comment into > >> the test source > >> > >> Changes in v3: > >> - added trivial implementation for kallsyms_lookup_name_in_module() for > >> !CONFIG_MODULES (noticed by test robot, fix suggested by Hao Luo) > >> > >> Changes in v2: > >> - introduced and used more space-efficient kallsyms lookup function, > >> suggested by Jiri Olsa > >> - included Hao Luo's comments > >> > >> > >> Viktor Malik (2): > >> bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules > >> bpf/selftests: Test fentry attachment to shadowed functions > >> > >> include/linux/bpf.h | 2 + > >> kernel/bpf/syscall.c | 6 + > >> kernel/bpf/trampoline.c | 28 ---- > >> kernel/bpf/verifier.c | 18 ++- > >> kernel/module/internal.h | 5 + > >> net/bpf/test_run.c | 5 + > >> .../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 + > >> .../bpf/prog_tests/module_attach_shadow.c | 128 ++++++++++++++++++ > >> 8 files changed, 169 insertions(+), 29 deletions(-) > >> create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c > >> > >> -- > >> 2.39.1 > >> > > >
