Messages in this thread | | | From | Song Liu <> | Subject | Re: [syzbot] possible deadlock in register_for_each_vma | Date | Wed, 31 Mar 2021 20:18:30 +0000 |
| |
> On Mar 31, 2021, at 9:59 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 03/28, Hillf Danton wrote: >> >> On Sat, 27 Mar 2021 18:53:08 Oleg Nesterov wrote: >>> Hi Hillf, >>> >>> it seems that you already understand the problem ;) I don't. >> >> It is simpler than you thought - I always blindly believe what syzbot >> reported is true before it turns out false as I am not smarter than it. >> Feel free to laugh loud. > > I am not going to laugh. I too think that lockdep is more clever than me. > >>> Could you explain in details how double __register is possible ? and how >> >> Taking another look at the report over five minutes may help more? > > No. I spent much, much more time time and I still can't understand your > patch which adds UPROBE_REGISTERING. Quite possibly your patch is fine, > just I am not smart enough. > > And I am a bit surprised you refused to help me. > >>> it connects to this lockdep report? >> >> Feel free to show the report is false and ignore my noise. > > Well, this particular report looks correct but false-positive to me, > _free_event() is not possible, but I can be easily wrong and we need > to shut up lockdep anyway... > > > ------------------------------------------------------------------------------- > Add more CC's. So, we have the following trace > > -> #0 (dup_mmap_sem){++++}-{0:0}: > check_prev_add kernel/locking/lockdep.c:2936 [inline] > check_prevs_add kernel/locking/lockdep.c:3059 [inline] > validate_chain kernel/locking/lockdep.c:3674 [inline] > __lock_acquire+0x2b14/0x54c0 kernel/locking/lockdep.c:4900 > lock_acquire kernel/locking/lockdep.c:5510 [inline] > lock_acquire+0x1ab/0x740 kernel/locking/lockdep.c:5475 > percpu_down_write+0x95/0x440 kernel/locking/percpu-rwsem.c:217 > register_for_each_vma+0x2c/0xc10 kernel/events/uprobes.c:1040 > __uprobe_register+0x5c2/0x850 kernel/events/uprobes.c:1181 > trace_uprobe_enable kernel/trace/trace_uprobe.c:1065 [inline] > probe_event_enable+0x357/0xa00 kernel/trace/trace_uprobe.c:1134 > trace_uprobe_register+0x443/0x880 kernel/trace/trace_uprobe.c:1461 > perf_trace_event_reg kernel/trace/trace_event_perf.c:129 [inline] > perf_trace_event_init+0x549/0xa20 kernel/trace/trace_event_perf.c:204 > perf_uprobe_init+0x16f/0x210 kernel/trace/trace_event_perf.c:336 > perf_uprobe_event_init+0xff/0x1c0 kernel/events/core.c:9754 > perf_try_init_event+0x12a/0x560 kernel/events/core.c:11071 > perf_init_event kernel/events/core.c:11123 [inline] > perf_event_alloc.part.0+0xe3b/0x3960 kernel/events/core.c:11403 > perf_event_alloc kernel/events/core.c:11785 [inline] > __do_sys_perf_event_open+0x647/0x2e60 kernel/events/core.c:11883 > do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > which shows that this path takes > > event_mutex -> uprobe.register_rwsem -> dup_mmap_sem -> mm.mmap_lock > > Not good. If nothing else, perf_mmap_close() path can take event_mutex under > mm.mmap_lock, so lockdep complains correctly. > > But why does perf_uprobe_init() take event_mutex? The comment mentions > uprobe_buffer_enable(). > > If this is the only reason, then why uprobe_buffer_enable/disable abuse > event_mutex? > > IOW, can something like the stupid patch below work? (Just in case... yes > it is very suboptimal, I am just trying to understand the problem). > > Song, Namhyung, Peter, what do you think? > > Oleg.
I think the following patch works well. I haven't tested it though.
Thanks, Song
> > --- x/kernel/trace/trace_event_perf.c > +++ x/kernel/trace/trace_event_perf.c > @@ -327,16 +327,9 @@ int perf_uprobe_init(struct perf_event *p_event, > goto out; > } > > - /* > - * local trace_uprobe need to hold event_mutex to call > - * uprobe_buffer_enable() and uprobe_buffer_disable(). > - * event_mutex is not required for local trace_kprobes. > - */ > - mutex_lock(&event_mutex); > ret = perf_trace_event_init(tp_event, p_event); > if (ret) > destroy_local_trace_uprobe(tp_event); > - mutex_unlock(&event_mutex); > out: > kfree(path); > return ret; > --- x/kernel/trace/trace_uprobe.c > +++ x/kernel/trace/trace_uprobe.c > @@ -857,6 +857,7 @@ struct uprobe_cpu_buffer { > }; > static struct uprobe_cpu_buffer __percpu *uprobe_cpu_buffer; > static int uprobe_buffer_refcnt; > +static DEFINE_MUTEX(uprobe_buffer_mutex); > > static int uprobe_buffer_init(void) > { > @@ -894,13 +895,13 @@ static int uprobe_buffer_enable(void) > { > int ret = 0; > > - BUG_ON(!mutex_is_locked(&event_mutex)); > - > + mutex_lock(&uprobe_buffer_mutex); > if (uprobe_buffer_refcnt++ == 0) { > ret = uprobe_buffer_init(); > if (ret < 0) > uprobe_buffer_refcnt--; > } > + mutex_unlock(&uprobe_buffer_mutex); > > return ret; > } > @@ -909,8 +910,7 @@ static void uprobe_buffer_disable(void) > { > int cpu; > > - BUG_ON(!mutex_is_locked(&event_mutex)); > - > + mutex_lock(&uprobe_buffer_mutex); > if (--uprobe_buffer_refcnt == 0) { > for_each_possible_cpu(cpu) > free_page((unsigned long)per_cpu_ptr(uprobe_cpu_buffer, > @@ -919,6 +919,7 @@ static void uprobe_buffer_disable(void) > free_percpu(uprobe_cpu_buffer); > uprobe_cpu_buffer = NULL; > } > + mutex_unlock(&uprobe_buffer_mutex); > } > > static struct uprobe_cpu_buffer *uprobe_buffer_get(void) >
| |