From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleg Nesterov Subject: Re: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't) Date: Tue, 21 Apr 2009 18:10:06 +0200 Message-ID: <20090421161006.GC5402@redhat.com> References: <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> <20090401030339.GX28946@ZenIV.linux.org.uk> <20090406153127.GA21220@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Al Viro , 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: Hugh Dickins Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On 04/19, Hugh Dickins wrote: > > On Mon, 6 Apr 2009, Oleg Nesterov wrote: > > > > Sorry for delay! > > Please don't suppose that you can ever beat me at the slowness game! I am still trying to compete... > > check_unsafe_exec() doesn't need ->siglock, we can iterate over sub-threads > > under rcu_read_lock(). Note that with RCU or ->siglock we can set the "wrong" > > LSM_UNSAFE_SHARE if we race with copy_process(CLONE_THREAD | CLONE_FS), but > > as it was already discussed we don't care. This means it is OK to miss the > > freshly cloned thread which has already passed copy_fs(). > > Yes, I agree. > And preferable not to have IRQs disabled over that next_thread() loop. Yes sure, we don't need local_irq_disable(), only rcu_read_lock(). > > T1 does clone(CLONE_FS /* without CLONE_THREAD */). > > > > T1 continues without LSM_UNSAFE_SHARE while ->fs is shared with another > > process. > > If I follow you correctly, you meant to say T2 not T1 in the last step. Yes, > Yes, I think your clear_in_exec change is a necessary one, > and your rcu_read_lock well worth while. OK, I'll send 2 simple patches, the first one kills lock_task_sighand(), another adds clear_in_exec. But, > One tiny change (aside from extending to compat_do_execve): > Al originally had check_unsafe_exec()'s write_lock(&p->fs->lock) > after the lock_task_sighand(p, &flags), but was forced to invert > that by the IRQ issue lockdep flagged. I think we'd all prefer > to think of fs->lock as an innermost lock, and would like it > now to go after your rcu_read_lock(). Since we are not going to disable IRQs, perhaps the above does not matter? It is always safe to take rcu_read_lock(), no matter which locks we already hold. > (You do rcu_read_unlock() earlier, but that's okay.) Yes, but unless we have a "strong" reason, it is better to take fs->lock first. rcu_read_lock() is free, but disables preemption. Oleg.