Re: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't)

[Oleg Nesterov <oleg@redhat.com>]

On 04/01, 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.
>
> 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.

Can't resist! I dislike the "in_exec && -EAGAIN" oddity too.

Yes sure, we can't break the "well written" applications. But imho this
looks strange. And a bit "assymetrical" wrt LSM_UNSAFE_SHARE, I mean
check_unsafe_exec() allows sub-threads to race or CLONE_FS but only if
LSM_UNSAFE_SHARE.

Another reason, we can have the "my test-case found something strange"
bug-reports.

So. Please feel free to nack or just ignore this message, but since I
personally dislike the current behaviour I should at least try to suggest
something else.

	- add "wait_queue_head_t in_exec_wait" to "struct linux_binprm".

	- kill fs->in_exec, add "wait_queue_head_t *in_exec_wait_ptr"
	  instead.

	- introduce the new helper,

		void fs_lock_check_exec(struct fs_struct *fs)
		{
			write_lock(&fs->lock);
			while (unlikely(fs->in_exec_wait_ptr)) {
				DECLARE_WAITQUEUE(wait, current);

				if (fatal_signal_pending(current))
					/*
					 * clone/exec can't succeed, and this
					 * thread won't return to the user-space
					 */
					break;

				__add_wait_queue(fs->in_exec_wait_ptr, &wait);
				__set_current_state(TASK_KILLABLE);
				write_unlock(&fs->lock);

				schedule();

				write_lock(&fs->lock);
				__remove_wait_queue(&wait);
			}
		}

	  Or we can return -EANYTHING when fatal_signal_pending(), this doesn't
	  matter.

	  Note that this helper can block only if we race with our sub-thread
	  in the middle of !LSM_UNSAFE_SHARE exec. Otherwise this is not slower
	  than write_lock(fs->lock) + if (fs->in_exec) we currently have.


	 - change copy_fs() to do

		if (clone_flags & CLONE_FS) {
			fs_lock_check_exec(fs);
			fs->users++;
			write_unlock(&fs->lock);
			return 0;
		}


	- change check_unsafe_exec:

		void check_unsafe_exec(struct linux_binprm *bprm)
		{
			struct task_struct *p = current, *t;
			unsigned n_fs;

			bprm->unsafe = tracehook_unsafe_exec(p);

			n_fs = 1;
			fs_lock_check_exec(&p->fs);
			if (p->fs->in_exec_wait_ptr)
				/* we are going to die */
				goto out;

			rcu_read_lock();
			for (t = next_thread(p); t != p; t = next_thread(t)) {
				if (t->fs == p->fs)
					n_fs++;
			}
			rcu_read_unlock();

			if (p->fs->users > n_fs) {
				bprm->unsafe |= LSM_UNSAFE_SHARE;
			} else {
				bprm->unsafe |= __LSM_EXEC_WAKE;
				init_waitqueue_head(&bprm->in_exec_wait);
				p->fs->in_exec_wait_ptr = &bprm->in_exec_wait;
			}
		out:
			write_unlock(&p->fs->lock);
		}



	 - and, finally, change do_execve()

			/* execve succeeded */
			current->fs->in_exec_wait_ptr = NULL;

			...

		out_unmark:
			if (bprm->unsafe & __LSM_EXEC_WAKE) {
				write_lock(&current->fs->lock);
				current->fs->in_exec_wait_ptr = NULL;
				wake_up_locked(&bprm->in_exec_wait);
				write_unlock(&current->fs->lock);
			}

Comments?

Oleg.