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

[Jiri Olsa <olsajiri@gmail.com>]

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
> > > 
> > 
>