From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753389AbbHMQTh (ORCPT ); Thu, 13 Aug 2015 12:19:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35019 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752122AbbHMQTg (ORCPT ); Thu, 13 Aug 2015 12:19:36 -0400 Date: Thu, 13 Aug 2015 18:17:18 +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: <20150813161718.GA23114@redhat.com> References: <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> <87vbcjyzac.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87vbcjyzac.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 On 08/13, Eric W. Biederman wrote: > > 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. Heh. so we still need to keep it. Thanks. > Yes. A shared sighand_struct will have a shared ->mm. But a private > sighand_struct with count == 1 may also have a shared ->mm. Yes sure. This just means that we can check current_is_single_threaded() if CLONE_SIGHAND | CLONE_VM, signal->count check can be avoided. > > 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. OK, > de_thread in fs/exec.c even seems to > rely on that. Not really. This is just optimization, de_thread() could change ->sighand unconditionally. > So while I agree with you that the sighand->count could suffer a similar > fate as mm_users it does not. Ignoring the out-of-tree code ;) Nevermind, I won't really argue, this all is mostly cosmetic. And perhaps this sighand->count check in check_unshare_flags() makes this code look a bit better / more understandable. Still. How about the trivial *-fix.patch for -mm which simply does - if (unshare_flags & (CLONE_SIGHAND | CLONE_VM)) { + if (unshare_flags & CLONE_SIGHAND) { if (atomic_read(¤t->sighand->count) > 1) return -EINVAL; } again, this doesn't really matter. To this "| CLONE_VM" looks very confusing to me. Oleg.