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. -- 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
