From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752216AbbHMM6J (ORCPT ); Thu, 13 Aug 2015 08:58:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41959 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751460AbbHMM6I (ORCPT ); Thu, 13 Aug 2015 08:58:08 -0400 Date: Thu, 13 Aug 2015 14:55:50 +0200 From: Oleg Nesterov To: "Eric W. Biederman" Cc: "Kirill A. Shutemov" , Andrew Morton , Kees Cook , David Howells , linux-kernel@vger.kernel.org, Peter Zijlstra , Ingo Molnar , "Kirill A. Shutemov" , Rik van Riel , Vladimir Davydov , Ricky Zhou , Julien Tinnes Subject: Re: [PATCH 1/2] unshare: Unsharing a thread does not require unsharing a vm Message-ID: <20150813125550.GA13984@redhat.com> References: <20150728221111.GA23391@node.dhcp.inet.fi> <20150805172356.GA20490@redhat.com> <87wpx9sjhq.fsf@x220.int.ebiederm.org> <87614tr2jd.fsf@x220.int.ebiederm.org> <20150806130629.GA4728@redhat.com> <20150806134426.GA6843@redhat.com> <871tf9cnbi.fsf_-_@x220.int.ebiederm.org> <87vbclb8op.fsf_-_@x220.int.ebiederm.org> <20150812174847.GA6703@redhat.com> <87614k73mo.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87614k73mo.fsf@x220.int.ebiederm.org> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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(¤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. 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.