proc: first_tid() fix/cleanup

proc_task_readdir() verifies that the result of get_proc_task() is pid_alive() and thus its ->group_leader is fine too. However this is not necessarily true after rcu_read_unlock(), we need to recheck this after first_tid() does rcu_read_lock() again.

The race is subtle and unlikely, but still it is possible afaics. To simplify lets ignore the "likely" case when tid != 0, f_version can be cleared by proc_task_operations->llseek().

Suppose we have a main thread M and its subthread T. Suppose that f_pos == 3, iow first_tid() should return T. Now suppose that the following happens between rcu_read_unlock() and rcu_read_lock():

1. T execs and becomes the new leader. This removes M from ->thread_group but next_thread(M) is still T.

2. T creates another thread X which does exec as well, T goes away.

3. X creates another subthread, this increments nr_threads.

4. first_tid() does next_thread(M) and returns the already dead T.

Note that we need 2. and 3. only because of get_nr_threads() check, and this check was supposed to be optimization only.

Note: I think that proc_task_readdir/first_tid interaction can be simplified, but this needs another patch. proc_task_readdir() should not play with ->group_leader at all. See the next patches.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

fs/proc/base.c +0 -0

index dd51e50..c939c9f 100644

fs/proc/base.c +3 -0

 	if (nr && nr >= get_nr_threads(leader))
 		goto out;
+	/* It could be unhashed before we take rcu lock */
+	if (!pid_alive(leader))
+		goto out;
 
 	/* If we haven't found our starting place yet start
 	 * with the leader and walk nr threads forward.

Quoted 4 lines · Oleg Nesterov

proc_task_readdir() verifies that the result of get_proc_task() is pid_alive() and thus its ->group_leader is fine too. However this is not necessarily true after rcu_read_unlock(), we need to recheck this after first_tid() does rcu_read_lock() again.

I agree with you but you are missing something critical from your explanation. If a process has been passed through __unhash_process then task->thread_group.next (aka next_thread) returns a pointer to the process that was it's next thread in the thread group. Importantly that pointer is only guaranteed to point to valid memory until the rcu grace period expires.

Quoted 2 lines

Note that we need 2. and 3. only because of get_nr_threads() check, and this check was supposed to be optimization only.

An optimization and denial of service attack prevention. It keeps us spinning for nearly unbounded amounts of time in the rcu critical section.

But I agree it should not be needed from this part of correctness.

fs/proc/base.c static struct task_struct *first_tid(struct task_struct *leader, · line 3190

Quoted 4 lines

fs/proc/base.c +1 -1

-	/* If nr exceeds the number of threads there is nothing todo */
 	pos = NULL;
+	/* If nr exceeds the number of threads there is nothing todo */

Moving the comment is just noise and makes for confusing reading of your patch.

proc_task_readdir() does not really need "leader", first_tid() has to revalidate it anyway. Just pass proc_pid(inode) to first_tid() instead, it can do pid_task(PIDTYPE_PID) itself and read ->group_leader only if necessary.

Note: I am not sure proc_task_readdir() really needs the initial -ENOENT check, but this is what the current code does.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c |   46 ++++++++++++++++------------------------------
 1 files changed, 16 insertions(+), 30 deletions(-)

fs/proc/base.c +0 -1

index bed1096..dbc4dae 100644
@@ -3173,34 +3173,35 @@ out_no_task:
  * In the case of a seek we start with the leader and walk nr
  * threads past it.
  */
-static struct task_struct *first_tid(struct task_struct *leader,

fs/proc/base.c +3 -15

 			goto found;
 	}
 
 	/* If nr exceeds the number of threads there is nothing todo */
-	if (nr && nr >= get_nr_threads(leader))
-		goto fail;
-	/* It could be unhashed before we take rcu lock */
-	if (!pid_alive(leader))
+	if (nr && nr >= get_nr_threads(task))
 		goto fail;
 
 	/* If we haven't found our starting place yet start
 	 * with the leader and walk nr threads forward.
 	 */
-	pos = leader;
+	pos = task = task->group_leader;
 	do {
 		if (nr-- <= 0)
 			goto found;
-	} while_each_thread(leader, pos);
+	} while_each_thread(task, pos);
 fail:
 	pos = NULL;
 	goto out;

@@ -3247,26 +3248,13 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi
 {
 	struct dentry *dentry = filp->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
-	struct task_struct *leader = NULL;
 	struct task_struct *task;
-	int retval = -ENOENT;
 	ino_t ino;
 	int tid;
 	struct pid_namespace *ns;
 
-	task = get_proc_task(inode);
-	if (!task)
-		goto out_no_task;
-	rcu_read_lock();
-	if (pid_alive(task)) {
-		leader = task->group_leader;
-		get_task_struct(leader);

fs/proc/base.c +0 -0

 	switch ((unsigned long)filp->f_pos) {

fs/proc/base.c +2 -4

 	case 0:

@@ -3289,7 +3277,7 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi
 	ns = filp->f_dentry->d_sb->s_fs_info;
 	tid = (int)filp->f_version;
 	filp->f_version = 0;
-	for (task = first_tid(leader, tid, filp->f_pos - 2, ns);
+	for (task = first_tid(proc_pid(inode), tid, filp->f_pos - 2, ns);
 	     task;
 	     task = next_tid(task), filp->f_pos++) {
 		tid = task_pid_nr_ns(task, ns);

@@ -3302,9 +3290,7 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi
 		}
 	}
 out:
-	put_task_struct(leader);
-out_no_task:
-	return retval;
+	return 0;
 }
 
 static int proc_task_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)

Quoted 5 lines · Oleg Nesterov

proc_task_readdir() does not really need "leader", first_tid() has to revalidate it anyway. Just pass proc_pid(inode) to first_tid() instead, it can do pid_task(PIDTYPE_PID) itself and read ->group_leader only if necessary.

Quoted 2 lines

Note: I am not sure proc_task_readdir() really needs the initial -ENOENT check, but this is what the current code does.

This looks like a nice cleanup.

We would need either -ENOENT or a return of 0 and an empty directory at the least. We need the check so that empty directories don't have "." and ".." entries.

fs/proc/base.c out_no_task: · line 3189

Quoted 4 lines

fs/proc/base.c +1 -1

 	if (tid && (nr > 0)) {
 		pos = find_task_by_pid_ns(tid, ns);
-		if (pos && (pos->group_leader == leader))
+		if (pos && same_thread_group(pos, task))

Sigh this reminds me we need to figure out how to kill task->pid and task->tgid,

which I assume means fixing same_thread_group.

fs/proc/base.c static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi · line 3257

Quoted 2 lines

fs/proc/base.c +2 -0

+	if (!pid_task(proc_pid(inode), PIDTYPE_PID))
+		return -ENOENT;

Strictly speaking this call to pid_task needs to be in a rcu critical section.

Quoted 13 lines · Eric W. Biederman

Quoted 7 lines · Oleg Nesterov

proc_task_readdir() does not really need "leader", first_tid() has to revalidate it anyway. Just pass proc_pid(inode) to first_tid() instead, it can do pid_task(PIDTYPE_PID) itself and read ->group_leader only if necessary.

Note: I am not sure proc_task_readdir() really needs the initial -ENOENT check, but this is what the current code does.

This looks like a nice cleanup.

Quoted 3 lines

We would need either -ENOENT or a return of 0 and an empty directory at the least. We need the check so that empty directories don't have "." and ".." entries.

And this is not clear to me...

Why the empty "." + ".." dir is bad if the task(s) has gone away after opendir?

Quoted 7 lines

fs/proc/base.c out_no_task: · line 3189

Quoted 4 lines

fs/proc/base.c +1 -1

 	if (tid && (nr > 0)) {
 		pos = find_task_by_pid_ns(tid, ns);
-		if (pos && (pos->group_leader == leader))
+		if (pos && same_thread_group(pos, task))

Sigh this reminds me we need to figure out how to kill task->pid and task->tgid,

Yeah.

Quoted 1 line

which I assume means fixing same_thread_group.

Now that ->signal can't go away before task_struct, we can make it

	static inline
	int same_thread_group(struct task_struct *p1, struct task_struct *p2)
	{
		return p1->signal == p2->signal;
	}

Quoted 5 lines

fs/proc/base.c static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi · line 3257

Quoted 2 lines

fs/proc/base.c +2 -0

+	if (!pid_task(proc_pid(inode), PIDTYPE_PID))
+		return -ENOENT;

Strictly speaking this call to pid_task needs to be in a rcu critical section.

Argh, thanks.

we do not really need rcu, we are not going to dereference this pointer, but we should make __rcu_dereference_check() happy...

I'll change this... but once again, can't we simply remove this check?

While you are here. Could you explain the ->d_inode check in proc_fill_cache() ? The code _looks_ wrong,

	if (!child || IS_ERR(child) || !child->d_inode)
		goto end_instantiate;

If d_inode == NULL, who does dput() ?

OTOH, if we ensure d_inode != NULL, why do we check "if (inode)" after inode = child->d_inode ?

IOW, it seems that this check should be simply removed?

Quoted 2 lines · Oleg Nesterov

Why the empty "." + ".." dir is bad if the task(s) has gone away after opendir?

Because the definition of a deleted directory that you are in is that getdents will return -ENOENT.

You can reproduce this with any linux filesystem.
mkdir foo
cd foo
rmdir ../foo
strace -f ls .

   open(".", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
   getdents(3, 0x1851c88, 32768)           = -1 ENOENT (No such file or directory)
   close(3)                                = 0

Eric, sorry for delay.

Quoted 15 lines · Eric W. Biederman

Quoted 2 lines · Oleg Nesterov

Why the empty "." + ".." dir is bad if the task(s) has gone away after opendir?

Because the definition of a deleted directory that you are in is that getdents will return -ENOENT.

You can reproduce this with any linux filesystem.
mkdir foo
cd foo
rmdir ../foo
strace -f ls .

Quoted 3 lines

   open(".", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
   getdents(3, 0x1851c88, 32768)           = -1 ENOENT (No such file or directory)
   close(3)                                = 0

Heh. Indeed, vfs_readdir() checks IS_DEADDIR().

Thanks.

OK. But this means that even 1/3 is not 100% right, exactly because leader can be unhashed right before first_tid() takes rcu lock. Easy to fix, we should simply factor out the "nr != 0" check.

And this also means that 3/3 is not right by the same reason. I'll make a simpler patch which only avoids the unnecessary get/put in proc_task_readdir().

Unless we can tolerate this very unlikely rase when the leader goes away after initial ENOENT check at the start, of course... Or unless we add canceldir() which resets getdents_callback->previous so that we could return ENOENT after filldir() was already called ;)

Quoted 4 lines · Oleg Nesterov

OK. But this means that even 1/3 is not 100% right, exactly because leader can be unhashed right before first_tid() takes rcu lock. Easy to fix, we should simply factor out the "nr != 0" check.

Quoted 8 lines

And this also means that 3/3 is not right by the same reason. I'll make a simpler patch which only avoids the unnecessary get/put in proc_task_readdir().

Unless we can tolerate this very unlikely rase when the leader goes away after initial ENOENT check at the start, of course... Or unless we add canceldir() which resets getdents_callback->previous so that we could return ENOENT after filldir() was already called ;)

A small race is fine and is fundamental to the process of readdir.

The guarantee of open+readdir+close is that all directory entries that exited before open and after close are returned. Directory entries that are added or removed during the open+readir+close are returned at most once.

The important case to handle is when someone has opened the directory a very long time ago or has chdir'd to the directory. With the result the directory was removed before we start the readdir process entirely.

If the tasks die in the narrow window while we are inside of readdir races are impossible to avoid.

Hello.

next_thread() should be avoided, probably next_tid() is the only "valid" user.

But now we have another reason to avoid (and probably even kill) it, we are going to replace or fix while_each_thread(), almost every lockless usage is wrong.

I was going to send more changes, but this initial series nearly killed me. And I think first_tid() needs another cleanup, ->f_pos truncation doesn't look nice, tomorrow.