Re: namespace: deadlock in dec_pid_namespaces

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]



On 20.01.2017 20:05, Eric W. Biederman wrote:
> Nikolay Borisov <n.borisov.lkml@gmail.com> writes:
> 
>> On 20.01.2017 15:07, Dmitry Vyukov wrote:
>>> Hello,
>>>
>>> I've got the following deadlock report while running syzkaller fuzzer
>>> on eec0d3d065bfcdf9cd5f56dd2a36b94d12d32297 of linux-next (on odroid
>>> device if it matters):
> 
> I am puzzled I thought we had fixed this with:
>   add7c65ca426 ("pid: fix lockdep deadlock warning due to ucount_lock")
> But apparently not.  We  just moved it from hardirq to softirq context.  Bah.
> 
> Thank you very much for the report.
> 
> Nikolay can you make your change use spinlock_irq?  And have put_ucounts
> do spin_lock_irqsave?  That way we just don't care where we call this.

Like the one attached? I haven't really taken careful look as to whether
the function where _irq versions do fiddle with irq state, since this
might cause a problem if we unconditionally enable them.

> 
> I a tired of being clever.
> 
> Eric
> 
> 

[-- Attachment #2: 0001-userns-Make-ucounts-lock-softirq-safe.patch --]
[-- Type: text/x-patch, Size: 2488 bytes --]

>From 0aa66c85afdac0cd07fabdf899c173c6dca2b6e7 Mon Sep 17 00:00:00 2001
From: Nikolay Borisov <n.borisov.lkml@gmail.com>
Date: Fri, 20 Jan 2017 15:21:35 +0200
Subject: [PATCH] userns: Make ucounts lock softirq-safe

The ucounts_lock is being used to protect various ucounts lifecycle
management functionalities. However, those services can also be invoked
when a pidns is being freed in an RCU callback (e.g. softirq context).
This can lead to deadlocks. There were already efforts trying to
prevent similar deadlocks in add7c65ca426 ("pid: fix lockdep deadlock
warning due to ucount_lock"), however they just moved the context
from hardirq to softrq. Fix this issue once and for all by explictly
making the lock disable irqs altogether.

Fixes: add7c65ca426 ("pid: fix lockdep deadlock warning due to ucount_lock")
Link: https://www.spinics.net/lists/kernel/msg2426637.html
Signed-off-by: Nikolay Borisov <n.borisov.lkml@gmail.com>
---
 kernel/ucount.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/ucount.c b/kernel/ucount.c
index b4aaee935b3e..68716403b261 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -132,10 +132,10 @@ static struct ucounts *get_ucounts(struct user_namespace *ns, kuid_t uid)
 	struct hlist_head *hashent = ucounts_hashentry(ns, uid);
 	struct ucounts *ucounts, *new;
 
-	spin_lock(&ucounts_lock);
+	spin_lock_irq(&ucounts_lock);
 	ucounts = find_ucounts(ns, uid, hashent);
 	if (!ucounts) {
-		spin_unlock(&ucounts_lock);
+		spin_unlock_irq(&ucounts_lock);
 
 		new = kzalloc(sizeof(*new), GFP_KERNEL);
 		if (!new)
@@ -145,7 +145,7 @@ static struct ucounts *get_ucounts(struct user_namespace *ns, kuid_t uid)
 		new->uid = uid;
 		atomic_set(&new->count, 0);
 
-		spin_lock(&ucounts_lock);
+		spin_lock_irq(&ucounts_lock);
 		ucounts = find_ucounts(ns, uid, hashent);
 		if (ucounts) {
 			kfree(new);
@@ -156,16 +156,18 @@ static struct ucounts *get_ucounts(struct user_namespace *ns, kuid_t uid)
 	}
 	if (!atomic_add_unless(&ucounts->count, 1, INT_MAX))
 		ucounts = NULL;
-	spin_unlock(&ucounts_lock);
+	spin_unlock_irq(&ucounts_lock);
 	return ucounts;
 }
 
 static void put_ucounts(struct ucounts *ucounts)
 {
+	unsigned long flags;
+
 	if (atomic_dec_and_test(&ucounts->count)) {
-		spin_lock(&ucounts_lock);
+		spin_lock_irqsave(&ucounts_lock, flags);
 		hlist_del_init(&ucounts->node);
-		spin_unlock(&ucounts_lock);
+		spin_unlock_irqrestore(&ucounts_lock, flags);
 
 		kfree(ucounts);
 	}
-- 
2.7.4