| Message ID | d4a7235586e3ca1b667f220de7b4835a1382397c.1670847888.git.vmalik@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix attaching fentry/fexit/fmod_ret/lsm to modules | expand |
On 12/12/22 4:59 AM, Viktor Malik wrote: > When attaching fentry/fexit/fmod_ret/lsm to a function located in a > module without specifying the target program, 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. > > This 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. > > Since the attachment must contain the BTF of the program to attach to, > we may extract the module from it and search for the function address in > the module. > > Signed-off-by: Viktor Malik <vmalik@redhat.com> > --- > kernel/bpf/verifier.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a5255a0dcbb6..d646c5263bc5 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -24,6 +24,7 @@ > #include <linux/bpf_lsm.h> > #include <linux/btf_ids.h> > #include <linux/poison.h> > +#include "../module/internal.h" > > #include "disasm.h" > > @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > const char *tname; > struct btf *btf; > long addr = 0; > + struct module *mod; > > if (!btf_id) { > bpf_log(log, "Tracing programs must provide btf_id\n"); > @@ -16645,7 +16647,19 @@ 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)) { > + preempt_disable(); > + mod = btf_try_get_module(btf); > + if (mod) { > + addr = find_kallsyms_symbol_value(mod, tname); > + module_put(mod); > + } else { > + addr = 0; > + } > + preempt_enable(); What if module is unloaded right after preempt_enabled so 'addr' becomes invalid? Is this a corner case we should consider? > + } else { > + addr = kallsyms_lookup_name(tname); > + } > if (!addr) { > bpf_log(log, > "The address of function %s cannot be found\n",
On 12/12/22 13:59, Viktor Malik wrote: > When attaching fentry/fexit/fmod_ret/lsm to a function located in a > module without specifying the target program, 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. > > This 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. > > Since the attachment must contain the BTF of the program to attach to, > we may extract the module from it and search for the function address in > the module. > > Signed-off-by: Viktor Malik <vmalik@redhat.com> > --- > kernel/bpf/verifier.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a5255a0dcbb6..d646c5263bc5 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -24,6 +24,7 @@ > #include <linux/bpf_lsm.h> > #include <linux/btf_ids.h> > #include <linux/poison.h> > +#include "../module/internal.h" Looks like there's a number of errors reported by test robot due to including "../module/internal.h" (mostly unrelated to this patchset). I'm not sure how to approach those - move find_kallsyms_symbol_value to include/linux/module.h or just ignore the errors? For both cases, a trivial find_kallsyms_symbol_value for !CONFIG_KALLSYMS will be necessary. > > #include "disasm.h" > > @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > const char *tname; > struct btf *btf; > long addr = 0; > + struct module *mod; > > if (!btf_id) { > bpf_log(log, "Tracing programs must provide btf_id\n"); > @@ -16645,7 +16647,19 @@ 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)) { > + preempt_disable(); > + mod = btf_try_get_module(btf); > + if (mod) { > + addr = find_kallsyms_symbol_value(mod, tname); > + module_put(mod); > + } else { > + addr = 0; > + } > + preempt_enable(); > + } else { > + addr = kallsyms_lookup_name(tname); > + } > if (!addr) { > bpf_log(log, > "The address of function %s cannot be found\n",
On 12/12/22 18:08, Yonghong Song wrote: > > > On 12/12/22 4:59 AM, Viktor Malik wrote: >> When attaching fentry/fexit/fmod_ret/lsm to a function located in a >> module without specifying the target program, 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. >> >> This 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. >> >> Since the attachment must contain the BTF of the program to attach to, >> we may extract the module from it and search for the function address in >> the module. >> >> Signed-off-by: Viktor Malik <vmalik@redhat.com> >> --- >> kernel/bpf/verifier.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index a5255a0dcbb6..d646c5263bc5 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -24,6 +24,7 @@ >> #include <linux/bpf_lsm.h> >> #include <linux/btf_ids.h> >> #include <linux/poison.h> >> +#include "../module/internal.h" >> #include "disasm.h" >> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct >> bpf_verifier_log *log, >> const char *tname; >> struct btf *btf; >> long addr = 0; >> + struct module *mod; >> if (!btf_id) { >> bpf_log(log, "Tracing programs must provide btf_id\n"); >> @@ -16645,7 +16647,19 @@ 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)) { >> + preempt_disable(); >> + mod = btf_try_get_module(btf); >> + if (mod) { >> + addr = find_kallsyms_symbol_value(mod, tname); >> + module_put(mod); >> + } else { >> + addr = 0; >> + } >> + preempt_enable(); > > What if module is unloaded right after preempt_enabled so 'addr' becomes > invalid? Is this a corner case we should consider? IIUC, if 'addr' becomes invalid, the attachment will eventually fail. So I'd say that there's no need to consider that case here, it's not considered for kallsyms_lookup_name below (which may call module_kallsyms_lookup_name) either. > >> + } else { >> + addr = kallsyms_lookup_name(tname); >> + } >> if (!addr) { >> bpf_log(log, >> "The address of function %s cannot be found\n", >
On 12/13/22 2:59 AM, Viktor Malik wrote: > On 12/12/22 18:08, Yonghong Song wrote: >> >> >> On 12/12/22 4:59 AM, Viktor Malik wrote: >>> When attaching fentry/fexit/fmod_ret/lsm to a function located in a >>> module without specifying the target program, 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. >>> >>> This 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. >>> >>> Since the attachment must contain the BTF of the program to attach to, >>> we may extract the module from it and search for the function address in >>> the module. >>> >>> Signed-off-by: Viktor Malik <vmalik@redhat.com> >>> --- >>> kernel/bpf/verifier.c | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index a5255a0dcbb6..d646c5263bc5 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -24,6 +24,7 @@ >>> #include <linux/bpf_lsm.h> >>> #include <linux/btf_ids.h> >>> #include <linux/poison.h> >>> +#include "../module/internal.h" >>> #include "disasm.h" >>> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct >>> bpf_verifier_log *log, >>> const char *tname; >>> struct btf *btf; >>> long addr = 0; >>> + struct module *mod; >>> if (!btf_id) { >>> bpf_log(log, "Tracing programs must provide btf_id\n"); >>> @@ -16645,7 +16647,19 @@ 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)) { >>> + preempt_disable(); >>> + mod = btf_try_get_module(btf); >>> + if (mod) { >>> + addr = find_kallsyms_symbol_value(mod, tname); >>> + module_put(mod); >>> + } else { >>> + addr = 0; >>> + } >>> + preempt_enable(); >> >> What if module is unloaded right after preempt_enabled so 'addr' >> becomes invalid? Is this a corner case we should consider? > > IIUC, if 'addr' becomes invalid, the attachment will eventually fail. > > So I'd say that there's no need to consider that case here, it's not > considered for kallsyms_lookup_name below (which may call > module_kallsyms_lookup_name) either. The below kallsyms_lookup_name(tname) works for vmlinux and vmlinux function won't go away. The following is what I understand for module address: bpf_tracing_prog_attach: ... bpf_check_attach_target (get addr etc. into tgt_info. ... bpf_trampoline_link_prog __bpf_trampoline_link_prog bpf_trampoline_update register_fentry bpf_trampoline_module_get static int bpf_trampoline_module_get(struct bpf_trampoline *tr) { struct module *mod; int err = 0; preempt_disable(); mod = __module_text_address((unsigned long) tr->func.addr); if (mod && !try_module_get(mod)) err = -ENOENT; preempt_enable(); tr->mod = mod; return err; } We try to grab a module reference here based on the func addr. But there is a risk that module (m1) might be unloaded and a different module (m2) might be loaded which occupies the address space of m1. In such cases, we might have issues. To resolve this issue, we might want to grab the reference earlier for find_kallsyms_symbol_value and won't release it until the program is detached. try_module_get() then will be unnecessary in this particular case. But care must be taken for other code paths. > >> >>> + } else { >>> + addr = kallsyms_lookup_name(tname); >>> + } >>> if (!addr) { >>> bpf_log(log, >>> "The address of function %s cannot be found\n", >> >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a5255a0dcbb6..d646c5263bc5 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -24,6 +24,7 @@ #include <linux/bpf_lsm.h> #include <linux/btf_ids.h> #include <linux/poison.h> +#include "../module/internal.h" #include "disasm.h" @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, const char *tname; struct btf *btf; long addr = 0; + struct module *mod; if (!btf_id) { bpf_log(log, "Tracing programs must provide btf_id\n"); @@ -16645,7 +16647,19 @@ 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)) { + preempt_disable(); + mod = btf_try_get_module(btf); + if (mod) { + addr = find_kallsyms_symbol_value(mod, tname); + module_put(mod); + } else { + addr = 0; + } + preempt_enable(); + } else { + addr = kallsyms_lookup_name(tname); + } if (!addr) { bpf_log(log, "The address of function %s cannot be found\n",
When attaching fentry/fexit/fmod_ret/lsm to a function located in a module without specifying the target program, 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. This 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. Since the attachment must contain the BTF of the program to attach to, we may extract the module from it and search for the function address in the module. Signed-off-by: Viktor Malik <vmalik@redhat.com> --- kernel/bpf/verifier.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)