Re: [PATCH 1/2] unshare: Unsharing a thread does not require unsharing a vm

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/11, Eric W. Biederman wrote:
>>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1866,13 +1866,17 @@ static int check_unshare_flags(unsigned long unshare_flags)
>>  				CLONE_NEWUSER|CLONE_NEWPID))
>>  		return -EINVAL;
>>  	/*
>> -	 * Not implemented, but pretend it works if there is nothing to
>> -	 * unshare. Note that unsharing CLONE_THREAD or CLONE_SIGHAND
>> -	 * needs to unshare vm.
>> +	 * Not implemented, but pretend it works if there is nothing
>> +	 * to unshare.  Note that unsharing the address space or the
>> +	 * signal handlers also need to unshare the signal queues (aka
>> +	 * CLONE_THREAD).
>>  	 */
>>  	if (unshare_flags & (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)) {
>> -		/* FIXME: get_task_mm() increments ->mm_users */
>> -		if (atomic_read(&current->mm->mm_users) > 1)
>> +		if (!thread_group_empty(current))
>> +			return -EINVAL;
>> +	}
>> +	if (unshare_flags & CLONE_VM) {
>> +		if (!current_is_single_threaded())
>>  			return -EINVAL;
>>  	}
>
> OK, but then you can remove "| CLONE_VM" from the previous check...

As an optimization, but I don't think anything cares enough for the
optimization to be worth the confusion.

>> @@ -1941,16 +1945,16 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
>>  	if (unshare_flags & CLONE_NEWUSER)
>>  		unshare_flags |= CLONE_THREAD | CLONE_FS;
>>  	/*
>> -	 * If unsharing a thread from a thread group, must also unshare vm.
>> -	 */
>> -	if (unshare_flags & CLONE_THREAD)
>> -		unshare_flags |= CLONE_VM;
>
> OK,
>
>>  	/*
>> +	 * If unsharing a signal handlers, must also unshare the signal queues.
>> +	 */
>> +	if (unshare_flags & CLONE_SIGHAND)
>> +		unshare_flags |= CLONE_THREAD;
>
> This looks unnecessary, check_unshare_flags() checks "THREAD | SIGHAND".
> And to me the comment looks misleading although I won't argue.

I absolutely can not understand this code if we jump 5 steps ahead
and optimize out the individual dependencies, and try for a flattened
dependency tree instead.  I can validate the individual dependencies
from first principles.

If we jump several steps ahead I can not validate the individual
dependencies.  

It really is important to say if you want your own private struct
sighand_struct, you also need to have your own private struct
signal_struct.

> And in fact this doesn't look exactly right, or I am totally confused.
> Shouldn't we do
>
> 	if (unshare_flags & CLONE_SIGHAND)
> 		unshare_flags |= CLONE_VM;

Nope.  The backward definitions of the flags in unshare has gotten you.
CLONE_SIGHAND means that you want a struct sighand_struct with a count
of 1.  Nothing about a sighand_struct with a count of 1 implies or
requires mm_users == 1.  clone can quite happily create those.

> ? Or change check_unshare_flags()...
>
> Otherwise suppose that a single threaded process does clone(VM | SIGHAND)
> and (say) child does sys_unshare(SIGHAND). This will wrongly succeed
> afaics.

Why would it be wrong to succeed in that case?  struct sighand_struct
has a count of 1.  unshare(CLONE_SIGHAND) requests a sighand_struct with
a count of 1.

I expect part of the confusion is the code in unshare has been wrongly
requiring an unshared vm to support a sighand_struct with a count of 1
since the day the code was merged.

Ugh. This patch has a bug where we don't check for sighand->count == 1.

clone(VM)  ---> mm_users = 2 sighand->count == 1 signal->live == 1

clone(VM|SIGHAND) --> mm_users = 2 sighand->count == 2 signal->live == 1

unshare(SIGHAND) needs to guarantee that when it returns sighand->count == 1.
So unshare(SIGHAND) needs to test for sighand->count == 1.

Ugh.  Untangling this ancient mess is a pain.  One more pass at this
patch it seems.

Eric