[PATCH v2 bpf] bpf: fix lockdep false positive in stackmap

[PATCH v2 bpf] bpf: fix lockdep false positive in stackmap

[Alexei Starovoitov]

Lockdep warns about false positive:
[   11.211460] ------------[ cut here ]------------
[   11.211936] DEBUG_LOCKS_WARN_ON(depth <= 0)
[   11.211985] WARNING: CPU: 0 PID: 141 at ../kernel/locking/lockdep.c:3592 lock_release+0x1ad/0x280
[   11.213134] Modules linked in:
[   11.214954] RIP: 0010:lock_release+0x1ad/0x280
[   11.223508] Call Trace:
[   11.223705]  <IRQ>
[   11.223874]  ? __local_bh_enable+0x7a/0x80
[   11.224199]  up_read+0x1c/0xa0
[   11.224446]  do_up_read+0x12/0x20
[   11.224713]  irq_work_run_list+0x43/0x70
[   11.225030]  irq_work_run+0x26/0x50
[   11.225310]  smp_irq_work_interrupt+0x57/0x1f0
[   11.225662]  irq_work_interrupt+0xf/0x20

since rw_semaphore is released in a different task vs task that locked the sema.
It is expected behavior.
Fix the warning with up_read_non_owner() and rwsem_release() annotation.

Fixes: bae77c5eb5b2 ("bpf: enable stackmap with build_id in nmi context")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/stackmap.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d43b14535827..950ab2f28922 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -44,7 +44,7 @@ static void do_up_read(struct irq_work *entry)
 	struct stack_map_irq_work *work;
 
 	work = container_of(entry, struct stack_map_irq_work, irq_work);
-	up_read(work->sem);
+	up_read_non_owner(work->sem);
 	work->sem = NULL;
 }
 
@@ -338,6 +338,12 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 	} else {
 		work->sem = &current->mm->mmap_sem;
 		irq_work_queue(&work->irq_work);
+		/*
+		 * The irq_work will release the mmap_sem with
+		 * up_read_non_owner(). The rwsem_release() is called
+		 * here to release the lock from lockdep's perspective.
+		 */
+		rwsem_release(&current->mm->mmap_sem.dep_map, 1, _RET_IP_);
 	}
 }
 
-- 
2.20.0

Re: [PATCH v2 bpf] bpf: fix lockdep false positive in stackmap

[Daniel Borkmann]

On 02/10/2019 09:52 PM, Alexei Starovoitov wrote:
> Lockdep warns about false positive:
> [   11.211460] ------------[ cut here ]------------
> [   11.211936] DEBUG_LOCKS_WARN_ON(depth <= 0)
> [   11.211985] WARNING: CPU: 0 PID: 141 at ../kernel/locking/lockdep.c:3592 lock_release+0x1ad/0x280
> [   11.213134] Modules linked in:
> [   11.214954] RIP: 0010:lock_release+0x1ad/0x280
> [   11.223508] Call Trace:
> [   11.223705]  <IRQ>
> [   11.223874]  ? __local_bh_enable+0x7a/0x80
> [   11.224199]  up_read+0x1c/0xa0
> [   11.224446]  do_up_read+0x12/0x20
> [   11.224713]  irq_work_run_list+0x43/0x70
> [   11.225030]  irq_work_run+0x26/0x50
> [   11.225310]  smp_irq_work_interrupt+0x57/0x1f0
> [   11.225662]  irq_work_interrupt+0xf/0x20
> 
> since rw_semaphore is released in a different task vs task that locked the sema.
> It is expected behavior.
> Fix the warning with up_read_non_owner() and rwsem_release() annotation.
> 
> Fixes: bae77c5eb5b2 ("bpf: enable stackmap with build_id in nmi context")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Applied, thanks!

Re: [PATCH v2 bpf] bpf: fix lockdep false positive in stackmap

[Eric Dumazet]

On Mon, Feb 11, 2019 at 7:39 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/10/2019 09:52 PM, Alexei Starovoitov wrote:
> > Lockdep warns about false positive:
> > [   11.211460] ------------[ cut here ]------------
> > [   11.211936] DEBUG_LOCKS_WARN_ON(depth <= 0)
> > [   11.211985] WARNING: CPU: 0 PID: 141 at ../kernel/locking/lockdep.c:3592 lock_release+0x1ad/0x280
> > [   11.213134] Modules linked in:
> > [   11.214954] RIP: 0010:lock_release+0x1ad/0x280
> > [   11.223508] Call Trace:
> > [   11.223705]  <IRQ>
> > [   11.223874]  ? __local_bh_enable+0x7a/0x80
> > [   11.224199]  up_read+0x1c/0xa0
> > [   11.224446]  do_up_read+0x12/0x20
> > [   11.224713]  irq_work_run_list+0x43/0x70
> > [   11.225030]  irq_work_run+0x26/0x50
> > [   11.225310]  smp_irq_work_interrupt+0x57/0x1f0
> > [   11.225662]  irq_work_interrupt+0xf/0x20
> >
> > since rw_semaphore is released in a different task vs task that locked the sema.
> > It is expected behavior.
> > Fix the warning with up_read_non_owner() and rwsem_release() annotation.
> >
> > Fixes: bae77c5eb5b2 ("bpf: enable stackmap with build_id in nmi context")
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> Applied, thanks!

Thanks everyone !