From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752464AbdATWo3 (ORCPT ); Fri, 20 Jan 2017 17:44:29 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34744 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752086AbdATWo2 (ORCPT ); Fri, 20 Jan 2017 17:44:28 -0500 Subject: Re: namespace: deadlock in dec_pid_namespaces To: "Eric W. Biederman" References: <877f5py5c2.fsf@xmission.com> Cc: Dmitry Vyukov , avagin , serge@hallyn.com, Kees Cook , Al Viro , LKML , syzkaller From: Nikolay Borisov Message-ID: Date: Sat, 21 Jan 2017 00:44:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <877f5py5c2.fsf@xmission.com> Content-Type: multipart/mixed; boundary="------------A6773B04CC39FE4700C5E305" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is a multi-part message in MIME format. --------------A6773B04CC39FE4700C5E305 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 20.01.2017 20:05, Eric W. Biederman wrote: > Nikolay Borisov 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 > > --------------A6773B04CC39FE4700C5E305 Content-Type: text/x-patch; name="0001-userns-Make-ucounts-lock-softirq-safe.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-userns-Make-ucounts-lock-softirq-safe.patch" >>From 0aa66c85afdac0cd07fabdf899c173c6dca2b6e7 Mon Sep 17 00:00:00 2001 From: Nikolay Borisov 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 --- 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 --------------A6773B04CC39FE4700C5E305--