netdev - Re: [PATCH bpf-next 1/2] bpf: enable stackmap with build

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30F32ACF-5155-459B-BD47-5060CCA52788@fb.com>
Date:   Wed, 2 May 2018 16:48:32 +0000
From:   Song Liu <songliubraving@...com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Networking <netdev@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next 1/2] bpf: enable stackmap with build_id in nmi
 context



> On May 2, 2018, at 2:21 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> On Tue, May 01, 2018 at 05:02:19PM -0700, Song Liu wrote:
>> @@ -267,17 +285,27 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>> {
>> 	int i;
>> 	struct vm_area_struct *vma;
>> +	bool in_nmi_ctx = in_nmi();
>> +	bool irq_work_busy = false;
>> +	struct stack_map_irq_work *work;
>> +
>> +	if (in_nmi_ctx) {
>> +		work = this_cpu_ptr(&irq_work);
>> +		if (work->sem)
>> +			/* cannot queue more up_read, fallback */
>> +			irq_work_busy = true;
>> +	}
>> 
>> 	/*
>> +	 * We cannot do up_read() in nmi context. To do build_id lookup
>> +	 * in nmi context, we need to run up_read() in irq_work. We use
>> +	 * a percpu variable to do the irq_work. If the irq_work is
>> +	 * already used by another lookup, we fall back to report ips.
>> 	 *
>> 	 * Same fallback is used for kernel stack (!user) on a stackmap
>> 	 * with build_id.
>> 	 */
>> +	if (!user || !current || !current->mm || irq_work_busy ||
>> 	    down_read_trylock(&current->mm->mmap_sem) == 0) {
>> 		/* cannot access current->mm, fall back to ips */
>> 		for (i = 0; i < trace_nr; i++) {
>> @@ -299,7 +327,13 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>> 			- vma->vm_start;
>> 		id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
>> 	}
>> +
>> +	if (!in_nmi_ctx)
>> +		up_read(&current->mm->mmap_sem);
>> +	else {
>> +		work->sem = &current->mm->mmap_sem;
>> +		irq_work_queue(&work->work);
>> +	}
>> }
> 
> This is disguisting.. :-)
> 
> It's broken though, I've bet you've never actually ran this with lockdep
> enabled for example.

I am not following here. I just run the new selftest with CONFIG_LOCKDEP on, 
and got no warning for this. 


> Also, you set work->sem before you do trylock, if the trylock fails you
> return early and keep work->sem set, which will thereafter always result
> in irq_work_busy.

work->sem was set after down_read_trylock(). I guess you misread the patch?

Thanks,
Song