From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753192AbbHMPo4 (ORCPT ); Thu, 13 Aug 2015 11:44:56 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:37373 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752455AbbHMPoy (ORCPT ); Thu, 13 Aug 2015 11:44:54 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Oleg Nesterov 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 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> <20150813125550.GA13984@redhat.com> Date: Thu, 13 Aug 2015 10:38:03 -0500 In-Reply-To: <20150813125550.GA13984@redhat.com> (Oleg Nesterov's message of "Thu, 13 Aug 2015 14:55:50 +0200") Message-ID: <87vbcjyzac.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1/oHjUOlPw4Z5DUQQ7FQtnFI0tF5x7zk4M= X-SA-Exim-Connect-IP: 67.3.205.173 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.7 XMSubLong Long Subject * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa05 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa05 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Oleg Nesterov X-Spam-Relay-Country: X-Spam-Timing: total 704 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 4.4 (0.6%), b_tie_ro: 3.6 (0.5%), parse: 0.74 (0.1%), extract_message_metadata: 3.6 (0.5%), get_uri_detail_list: 2.3 (0.3%), tests_pri_-1000: 3.3 (0.5%), tests_pri_-950: 1.03 (0.1%), tests_pri_-900: 0.82 (0.1%), tests_pri_-400: 25 (3.5%), check_bayes: 24 (3.4%), b_tokenize: 8 (1.1%), b_tok_get_all: 8 (1.2%), b_comp_prob: 2.3 (0.3%), b_tok_touch_all: 3.2 (0.5%), b_finish: 0.73 (0.1%), tests_pri_0: 654 (92.9%), tests_pri_500: 3.2 (0.5%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 1/2] unshare: Unsharing a thread does not require unsharing a vm X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Oleg Nesterov writes: > 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(). The only way killing CLONE_SIGHAND would be viable would be with a config option. There are entire generations of linux where libpthreads used this before CLONE_THREAD was implemented. Now perhaps no one cares anymore, but there are a lot of historic binairies that used it, even to the point where I know of at least one user outside of glibc's pthread implementation. > 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. Yes. A shared sighand_struct will have a shared ->mm. But a private sighand_struct with count == 1 may also have a shared ->mm. >> > 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. Now that I am clear about what you are talking about I agree with you. My apologies I clearly misread what you were saying yesterday. >> 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... I have verified that copy_sighand is the only place in the kernel where we increment sighand->count today. de_thread in fs/exec.c even seems to rely on that. So while I agree with you that the sighand->count could suffer a similar fate as mm_users it does not. Eric