Re: [PATCH bpf-next v8 0/2] Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Jiri Olsa <olsajiri@gmail.com>]

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@kernel.org>

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
>