Please add linux-modules in the future. My review below.
On Fri, Mar 10, 2023 at 08:40:59AM +0100, Viktor Malik wrote:
> This resolves two problems with attachment of fentry/fexit/fmod_ret/lsm
> to functions located in modules:
>
> 1. The verifier tries to find the address to attach to in kallsyms. This
> is always done by searching the entire kallsyms, not respecting the
> module in which the function is located. Such approach causes an
> incorrect attachment address to be computed if the function to attach
> to is shadowed by a function of the same name located earlier in
> kallsyms.
Just a heads up, I realize you have tried to fix the issue here using
semantics to get the module in other ways, but Nick's future work
expands kallsyms to allow symbols for modules to be looked for, even
if there are duplicates in kallsysms. He is working on that to help
with tooling for tracing, but here seems to be an example use case
which may be in-kernel. But you seem to indicate you've found an alternative
solution anyway.
Jiri, does live patching hunt for symbols in such opaque way too?
Or does it resolve it using something similar as the technique here
with a module notifier / its own linked list?
> 2. If the address to attach to is located in a module, the module
> reference is only acquired in register_fentry. If the module is
> unloaded between the place where the address is found
> (bpf_check_attach_target in the verifier) and register_fentry, it is
> possible that another module is loaded to the same address which may
> lead to potential errors.
>
> Since the attachment must contain the BTF of the program to attach to,
> we extract the module from it and search for the function address in the
> correct module (resolving problem no. 1). Then, the module reference is
> taken directly in bpf_check_attach_target and stored in the bpf program
> (in bpf_prog_aux). The reference is only released when the program is
> unloaded (resolving problem no. 2).
>
> Signed-off-by: Viktor Malik <vmalik@xxxxxxxxxx>
> Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
My review of the critical part below.
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 45a082284464..3905bb20b9a1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18432,8 +18434,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> else
> addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
> } else {
> - addr = kallsyms_lookup_name(tname);
> + if (btf_is_module(btf)) {
> + mod = btf_try_get_module(btf);
> + if (mod)
> + addr = find_kallsyms_symbol_value(mod, tname);
> + else
> + addr = 0;
> + } else {
> + addr = kallsyms_lookup_name(tname);
> + }
> if (!addr) {
> + module_put(mod);
> bpf_log(log,
> "The address of function %s cannot be found\n",
> tname);
If btf_modules linked list is ensured to not remove the btf module
during this operation, sure this is safe, as per the new guidelines I've
posted for try_module_get() this seems to be using try_module_get()
using the implied protection.
Please review the docs. *If* it respects that usage then feel free to
add:
Reviewed-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
Luis
