Oleg Nesterov <o...@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(¤t->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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
