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 > --- > 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 >
