Re: [syzbot] possible deadlock in register_for_each_vma

[Song Liu]

> On Mar 31, 2021, at 9:59 AM, Oleg Nesterov <oleg@xxxxxxxxxx> 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)
>