Re: [PATCH v2] unshare: Unsharing a thread does not require unsharing a vm

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 v2] unshare: Unsharing a thread does not require unsharing a vm
Date: Thu, 13 Aug 2015 11:01:46 -0500	[thread overview]
Message-ID: <87k2szw51x.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20150813125704.GB13984@redhat.com> (Oleg Nesterov's message of "Thu, 13 Aug 2015 14:57:04 +0200")

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/12, Eric W. Biederman wrote:
>>
>> +	if (unshare_flags & (CLONE_SIGHAND | CLONE_VM)) {
>> +		if (atomic_read(&current->sighand->count) > 1)
>> +			return -EINVAL;
>> +	}
>
> I am still not sure we want this... please the the previous email.

Reading your other email I did not see why you thought this check was
unnecessary.

> But perhaps I missed something.

In short:
clone(VM) --> mm_users > 1 && sighand_struct->count == 1
followed by:
unshare(SIGHAND)
the unshare should succeed.

Meanwhile:
clone(VM|SIGHAND) --> mm_users > 1 && sighand_struct->count > 1
followed by:
unshare(SIGHAND)
the unshare should fail.

I actually tested both of these cases and my patch works properly.

Not that I expect that there is anyone actually calling unshare(SIGHAND)
but unless we figure out how to remove the code, the code should
function correctly.  If for no other reason than to not confuse people
reading and maintaining the code.

Further I have audited the callers and we don't have anyone playing
games with sighand->count.  There is an implementation of unsharing the
sighand_struct in dethread in fs/exec.c that relies on this.

Other possible tests such as current_is_single_threaded() and
thread_group_empty() fail at the wrong times to be used.

So I think it is clear that testing for a private sighand_struct is
necessaring and testing sighand->count is a perfectly fine test.

Eric