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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
