From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hugh Dickins Subject: Re: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't) Date: Wed, 1 Apr 2009 12:18:11 +0100 (BST) Message-ID: References: <20090329235639.GA32199@redhat.com> <20090330000338.GB32199@redhat.com> <20090330010843.GM28946@ZenIV.linux.org.uk> <20090330011303.GN28946@ZenIV.linux.org.uk> <20090330013612.GA4080@redhat.com> <20090330014040.GA4807@redhat.com> <20090330123101.GQ28946@ZenIV.linux.org.uk> <20090331061615.GS28946@ZenIV.linux.org.uk> <20090401023849.GW28946@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Oleg Nesterov , Linus Torvalds , Andrew Morton , Joe Malicki , Michael Itz , Kenneth Baker , Chris Wright , David Howells , Alexey Dobriyan , Greg Kroah-Hartman , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Al Viro Return-path: Received: from [172.20.16.254]([172.20.16.254]) (3734 bytes) by megami.veritas.com via sendmail with P:esmtp/R:smart_host/T:smtp (sender: ) id for ; Wed, 1 Apr 2009 04:17:24 -0700 (PDT) (Smail-3.2.0.101 1997-Dec-17 #15 built 2001-Aug-30) In-Reply-To: <20090401023849.GW28946@ZenIV.linux.org.uk> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, 1 Apr 2009, Al Viro wrote: > On Wed, Apr 01, 2009 at 01:28:01AM +0100, Hugh Dickins wrote: > > > Otherwise it looks good to me, except I keep worrying about those > > EAGAINs. The more so once I noticed current->cred_exec_mutex is > > already being used to handle a similar issue with ptrace. What > > do you think of this rather smaller patch? which I'd much rather > > send after having slept on it, since it may be embarrassingly and > > obviously wrong, but tomorrow may be too late ... > > Eh... I'm not particulary happy with fork() growing heavier and heavier. I don't see it as making fork() any heavier, but never mind. The important thing is to get a fix out. > Besides, there's a subtle problem avoided by another variant - think what > happens if past the point of no return execve() will unshare fs_struct > (e.g. by explicit unshare() from dynamic linker). You're too far ahead of me there. > > Frankly, -EAGAIN in situation when we have userland race is fine. And > we *do* have a userland race here - execve() will kill -9 those threads > in case of success, so if they'd been doing something useful, they are > about to be suddenly screwed. Good point. I found it quite odd the way the awkward case (shared beyond the threadgroup) is allowed to go forward (with possibility that setuid will be undone), but the easy case is -EAGAINed. (And I gave up on trying to find a better name for your "in_exec" flag, which is rather more subtle than just that!) But odd as it is, there's good reason for doing it that way. > > So I stand by my variant. Fair enough. > Note that if we have *other* tasks sharing > fs_struct, your variant will block their clone() for the duration of > execve() while mine will simply leave them alone (and accept that we > have unsafe sharing). Yes, intentional, consistent with the existing cred_exec_mutex technique. Hugh