On Thu, Feb 16, 2023 at 04:50:41PM +0100, Jiri Olsa wrote:
> 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
It does, but it reenables preemption right away so it is enabled by the
time we call find_kallsyms_symbol_value(). I am getting the following
lockdep splat while running module_fentry_shadow test from test_progs.
[ 12.017973][ T488] =============================
[ 12.018529][ T488] WARNING: suspicious RCU usage
[ 12.018987][ T488] 6.2.0.bpf-test-13063-g6a9f5cdba3c5 #804 Tainted: G OE
[ 12.019898][ T488] -----------------------------
[ 12.020391][ T488] kernel/module/kallsyms.c:448 suspicious rcu_dereference_check() usage!
[ 12.021335][ T488]
[ 12.021335][ T488] other info that might help us debug this:
[ 12.021335][ T488]
[ 12.022416][ T488]
[ 12.022416][ T488] rcu_scheduler_active = 2, debug_locks = 1
[ 12.023297][ T488] no locks held by test_progs/488.
[ 12.023854][ T488]
[ 12.023854][ T488] stack backtrace:
[ 12.024336][ T488] CPU: 0 PID: 488 Comm: test_progs Tainted: G OE 6.2.0.bpf-test-13063-g6a9f5cdba3c5 #804
[ 12.025290][ T488] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
[ 12.026108][ T488] Call Trace:
[ 12.026381][ T488] <TASK>
[ 12.026649][ T488] dump_stack_lvl+0xb4/0x110
[ 12.027060][ T488] lockdep_rcu_suspicious+0x158/0x1f0
[ 12.027541][ T488] find_kallsyms_symbol_value+0xe8/0x110
[ 12.028028][ T488] bpf_check_attach_target+0x838/0xa20
[ 12.028511][ T488] check_attach_btf_id+0x144/0x3f0
[ 12.028957][ T488] ? __pfx_cmp_subprogs+0x10/0x10
[ 12.029408][ T488] bpf_check+0xeec/0x1850
[ 12.029799][ T488] ? ktime_get_with_offset+0x124/0x1d0
[ 12.030247][ T488] bpf_prog_load+0x87a/0xed0
[ 12.030627][ T488] ? __lock_release+0x5f/0x160
[ 12.031010][ T488] ? __might_fault+0x53/0xb0
[ 12.031394][ T488] ? selinux_bpf+0x6c/0xa0
[ 12.031756][ T488] __sys_bpf+0x53c/0x1240
[ 12.032115][ T488] __x64_sys_bpf+0x27/0x40
[ 12.032476][ T488] do_syscall_64+0x3e/0x90
[ 12.032835][ T488] entry_SYSCALL_64_after_hwframe+0x72/0xdc
[ 12.033313][ T488] RIP: 0033:0x7f174ea0e92d
[ 12.033668][ T488] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d3 e4 0c 00 f7 d8 64 89 0
1 48
[ 12.035197][ T488] RSP: 002b:00007ffee5cefc68 EFLAGS: 00000202 ORIG_RAX: 0000000000000141
[ 12.035864][ T488] RAX: ffffffffffffffda RBX: 00007ffee5cf02a8 RCX: 00007f174ea0e92d
[ 12.036495][ T488] RDX: 0000000000000080 RSI: 00007ffee5cefd20 RDI: 0000000000000005
[ 12.037123][ T488] RBP: 00007ffee5cefc80 R08: 00007ffee5cefea0 R09: 00007ffee5cefd20
[ 12.037752][ T488] R10: 0000000000000002 R11: 0000000000000202 R12: 0000000000000000
[ 12.038382][ T488] R13: 00007ffee5cf02c8 R14: 0000000000f2edb0 R15: 00007f174eb59000
[ 12.039022][ T488] </TASK>
> 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
> > > >
> > >
> >
--
Artem
