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

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
>> >
>> > > From: Ricky Zhou <rickyz@xxxxxxxxxxxx>
>> > >
>> > > 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@xxxxxxxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/