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

[Oleg Nesterov <oleg@redhat.com>]

Let me first say that CLONE_SIGHAND must die, I think ;) and perhaps
even sighand_struct... I am wondering if we can add something like

	if ((clone_flags & (CLONE_THREAD | CLONE_SIGHAND)) == CLONE_SIGHAND)
		pr_info("You are crazy, please report this to lkml\n");

into copy_process().

On 08/12, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 08/11, Eric W. Biederman wrote:
> >>  	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.

current_is_single_threaded() checks task->signal->live at the start,
so there is no optimization. But I won't argue, this doesn't hurt.

> >>  	/*
> >> +	 * 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.

OK,

> > 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.

See below,

> CLONE_SIGHAND means that you want a struct sighand_struct with a count
> of 1.

This is (almost) true,

> Nothing about a sighand_struct with a count of 1 implies or
> requires mm_users == 1.  clone can quite happily create those.

See

	if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))

in copy_process(). So if you have a shared sighand_struct, your ->mm
is also shared, current_is_single_threaded() will notice this.

> > 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.

How that? clone(VM | SIGHAND) will share ->sighand and increment its
count.

> unshare(CLONE_SIGHAND) requests a sighand_struct with
> a count of 1.

Exactly, that is why it is wrong to succeed.

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

Oh, I do not think we should check sighand->count. This can lead to
the same problem we have with the current current->mm->mm_users check.

Most probably today nobody increments sighand->count (I didn't even
try to verify). But this is possible, and I saw the code which did
this to pin ->sighand...

Oleg.