On Wed, Feb 22, 2023 at 03:35:27PM +0100, Viktor Malik 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. > > --- > Changes in v8: > - added module_put to error paths in bpf_check_attach_target after the > module reference is acquired I sent 2 other comments, but other than that it looks good Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx> jirka > > 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 | 27 ---- > 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(+), 28 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c > > -- > 2.39.1 >
