Re: [PATCH] user_ns: use correct check for single-threadedness

From: ebiederm@xmission.com (Eric W. Biederman)
To: Oleg Nesterov <oleg@redhat.com>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>,
	Andrew Morton <akpm@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	David Howells <dhowells@redhat.com>,
	linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Rik van Riel <riel@redhat.com>,
	Vladimir Davydov <vdavydov@parallels.com>,
	Ricky Zhou <rickyz@chromium.org>, Julien Tinnes <jln@google.com>
Subject: Re: [PATCH] user_ns: use correct check for single-threadedness
Date: Wed, 05 Aug 2015 13:00:49 -0500	[thread overview]
Message-ID: <87wpx9sjhq.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20150805172356.GA20490@redhat.com> (Oleg Nesterov's message of "Wed, 5 Aug 2015 19:23:56 +0200")

Oleg Nesterov <oleg@redhat.com> writes:

> On 07/29, Kirill A. Shutemov wrote:
>>
>> On Tue, Jul 28, 2015 at 02:35:04PM -0700, Andrew Morton wrote:
>> > On Tue, 28 Jul 2015 10:15:00 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >
>> > > From: Ricky Zhou <rickyz@chromium.org>
>> > >
>> > > Checking mm_users > 1 does not mean a process is multithreaded. For
>> > > example, reading /proc/PID/maps temporarily increments mm_users, allowing
>> > > other processes to (accidentally) interfere with unshare() calls.
>> > >
>> > > This fixes observed failures of unshare(CLONE_NEWUSER) incorrectly
>> > > returning EINVAL if another processes happened to be simultaneously
>> > > reading the maps file.
>> >
>> > Yikes.  current_is_single_threaded() is expensive.  Are we sure this
>> > isn't going to kill someone's workload?
>>
>> It's expensive only if mm_users > 1. We will go to for_each_process() only
>> if somebody outside of the process grabs mm_users references (like reading
>> /proc/PID/maps).
>
> Yes,
>
>> Or if it called it from multithreaded application.
>
> Not really, please note the "negative fast-path" check:
>
> 	if (atomic_read(&task->signal->live) != 1)	
> 		return false;
>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>
> Same here, I think the patch is fine.
>
> I don't think that sys_setns() is performance critical, and Eric seems
> to agree with this change too.

I do and I am about to merge it into my userns tree.  It is clearly an
issue that is affecting users in the real world, which fundamentally
makes the current implementation buggy.

That said the performance of sys_setns does matter.  Removing the
possibility of calling synchronize_rcu from switch_task_namespaces
happened because the performance of sys_setns was a problem.

Looking at the implementation of current_is_single_threaded() it appears
that the data structures dealing with threads could stand some
improvement so current_is_single_threaded() could become a constant time
function.

The class of application that is likely to call setns and be strongly
affected by it's performance are container management applications or
applications that are deliberately using more than one namespace at a
time.  There might be advantages to using current_is_single_threaded()
as currently written to slow down an application.  That slow down could
expand a race condition enough to make another bug exploitable.

So if we could figure out how to make current_is_single_threaded()
faster or at least have the slow path not triggerable by users playing
with proc I would very much be in favor of it.

Eric