On Thu, Feb 16, 2023 at 03:45:11PM +0100, Viktor Malik wrote:
SNIP
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 388245e8826e..6a19bd450558 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"
> > > @@ -16868,6 +16869,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > const char *tname;
> > > struct btf *btf;
> > > long addr = 0;
> > > + struct module *mod = NULL;
> > > if (!btf_id) {
> > > bpf_log(log, "Tracing programs must provide btf_id\n");
> > > @@ -17041,7 +17043,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)) {
> > > + preempt_disable();
> >
> > btf_try_get_module takes mutex, so you can't preempt_disable in here,
> > I got this when running the test:
> >
> > [ 691.916989][ T2585] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
> >
>
> Hm, do we even need to preempt_disable? IIUC, preempt_disable is used
> in module kallsyms to prevent taking the module lock b/c kallsyms are
> used in the oops path. That shouldn't be an issue here, is that correct?
btf_try_get_module calls try_module_get which disables the preemption,
so no need to call it in here
jirka
>
> > > + mod = btf_try_get_module(btf);
> > > + if (mod)
> > > + addr = find_kallsyms_symbol_value(mod, tname);
> > > + 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",
> > > @@ -17105,6 +17117,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > tgt_info->tgt_addr = addr;
> > > tgt_info->tgt_name = tname;
> > > tgt_info->tgt_type = t;
> > > + if (mod) {
> > > + if (!prog->aux->mod)
> > > + prog->aux->mod = mod;
> >
> > can this actually happen? would it be better to have bpf_check_attach_target
> > just to take take the module ref and return it in tgt_info->tgt_mod and it'd
> > be up to caller to decide what to do with that
>
> Ok, I'll try to do it that way.
>
> Thanks for the review!
> Viktor
>
> >
> > thanks,
> > jirka
> >
> > > + else
> > > + module_put(mod);
> > > + }
> > > return 0;
> > > }
> > > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > > index 2e2bf236f558..5cb103a46018 100644
> > > --- a/kernel/module/internal.h
> > > +++ b/kernel/module/internal.h
> > > @@ -256,6 +256,11 @@ static inline bool sect_empty(const Elf_Shdr *sect)
> > > static inline void init_build_id(struct module *mod, const struct load_info *info) { }
> > > static inline void layout_symtab(struct module *mod, struct load_info *info) { }
> > > static inline void add_kallsyms(struct module *mod, const struct load_info *info) { }
> > > +static inline unsigned long find_kallsyms_symbol_value(struct module *mod
> > > + const char *name)
> > > +{
> > > + return 0;
> > > +}
> > > #endif /* CONFIG_KALLSYMS */
> > > #ifdef CONFIG_SYSFS
> > > --
> > > 2.39.1
> > >
> >
>
