proc: first_tid() fix/cleanup

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.

The patch also extracts the "inode is dead" code
from pid_delete_dentry(dentry) into the new trivial helper, proc_inode_is_dead(inode), proc_task_readdir() uses it to return -ENOENT if this dir was removed. This is a bit racy, but the race is very inlikely and the getdents() after openndir() can see the empty "." + ".." dir only once.

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

---
 fs/proc/base.c |   53 ++++++++++++++++++++++-------------------------------
 1 files changed, 22 insertions(+), 31 deletions(-)

fs/proc/base.c +22 -31

index bed1096..5e0e02f 100644
@@ -1652,13 +1652,18 @@ int pid_revalidate(struct dentry *dentry, unsigned int flags)
 	return 0;
 }
 
+static inline bool proc_inode_is_dead(struct inode *inode)
+{
+	return !proc_pid(inode)->tasks[PIDTYPE_PID].first;
+}
+
 int pid_delete_dentry(const struct dentry *dentry)
 {
 	/* Is the task we represent dead?
 	 * If so, then don't put the dentry on the lru list,
 	 * kill it immediately.
 	 */
-	return !proc_pid(dentry->d_inode)->tasks[PIDTYPE_PID].first;
+	return proc_inode_is_dead(dentry->d_inode);
 }
 
 const struct dentry_operations pid_dentry_operations =

@@ -3173,34 +3178,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,
-		int tid, int nr, struct pid_namespace *ns)
+static struct task_struct *first_tid(struct pid *pid, int tid,
+					int nr, struct pid_namespace *ns)
 {
-	struct task_struct *pos;
+	struct task_struct *pos, *task;
 
 	rcu_read_lock();
-	/* Attempt to start with the pid of a thread */
+	task = pid_task(pid, PIDTYPE_PID);
+	if (!task)
+		goto fail;
+
+	/* Attempt to start with the tid of a thread */
 	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))
 			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 +3253,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);
-	}
-	rcu_read_unlock();
-	put_task_struct(task);
-	if (!leader)
-		goto out_no_task;
-	retval = 0;
+	if (proc_inode_is_dead(inode))
+		return -ENOENT;
 
 	switch ((unsigned long)filp->f_pos) {
 	case 0:

@@ -3289,7 +3282,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 +3295,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)

1. proc_task_readdir() truncates f_pos to long, this can lead to wrong result on 32bit.

2. first_tid() truncates f_pos to int, this is wrong even on 64bit.

We could check that f_pos < PID_MAX or even INT_MAX in proc_task_readdir(), but this patch simply checks the potential overflow in first_tid(), this check is nop on 64bit. We do not care if it was negative and the new unsigned value is huge, all we need to ensure is that we never wrongly return !NULL.

3. Remove the 2nd "nr != 0" check before get_nr_threads(), nr_threads == 0 is not distinguishable from !pid_task() above.

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

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

fs/proc/base.c +10 -6

index 5e0e02f..5598cfa 100644
@@ -3178,10 +3178,14 @@ 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 pid *pid, int tid,
-					int nr, struct pid_namespace *ns)
+static struct task_struct *first_tid(struct pid *pid, int tid, loff_t f_pos,
+					struct pid_namespace *ns)
 {
 	struct task_struct *pos, *task;
+	unsigned long nr = f_pos;
+
+	if (nr != f_pos)	/* 32bit overflow? */
+		return NULL;
 
 	rcu_read_lock();
 	task = pid_task(pid, PIDTYPE_PID);

@@ -3189,14 +3193,14 @@ static struct task_struct *first_tid(struct pid *pid, int tid,
 		goto fail;
 
 	/* Attempt to start with the tid of a thread */
-	if (tid && (nr > 0)) {
+	if (tid && nr) {
 		pos = find_task_by_pid_ns(tid, ns);
 		if (pos && same_thread_group(pos, task))
 			goto found;
 	}
 
 	/* If nr exceeds the number of threads there is nothing todo */
-	if (nr && nr >= get_nr_threads(task))
+	if (nr >= get_nr_threads(task))
 		goto fail;
 
 	/* If we haven't found our starting place yet start

@@ -3204,7 +3208,7 @@ static struct task_struct *first_tid(struct pid *pid, int tid,
 	 */
 	pos = task = task->group_leader;
 	do {
-		if (nr-- <= 0)
+		if (!nr--)
 			goto found;
 	} while_each_thread(task, pos);
 fail:

@@ -3261,7 +3265,7 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi
 	if (proc_inode_is_dead(inode))
 		return -ENOENT;
 
-	switch ((unsigned long)filp->f_pos) {
+	switch (filp->f_pos) {
 	case 0:
 		ino = inode->i_ino;
 		if (filldir(dirent, ".", 1, filp->f_pos, ino, DT_DIR) < 0)

1. proc_task_readdir() truncates f_pos to long, this can lead to wrong result on 32bit.

2. first_tid() truncates f_pos to int, this is wrong even on 64bit.

We could check that f_pos < PID_MAX or even INT_MAX in proc_task_readdir(), but this patch simply checks the potential overflow in first_tid(), this check is nop on 64bit. We do not care if it was negative and the new unsigned value is huge, all we need to ensure is that we never wrongly return !NULL.

3. Remove the 2nd "nr != 0" check before get_nr_threads(), nr_threads == 0 is not distinguishable from !pid_task() above.

Quoted 3 lines · Oleg Nesterov

1. proc_task_readdir() truncates f_pos to long, this can lead to wrong result on 32bit.

This won't compile on some 32bit architectures like x86-32.

switch(unsigned long long) requires helpers that the kernel does not included. Or at least the kernel has not included because such code is a problem. In fact that is the reason Linus put the case to unsigned long in there.

There is another bug in here as well that we may return really crazy things in the case of seek simultaneous with readdir.

I think we really need to put f_pos
in a local variable.

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.

Changes:

	1/4: Update the changelog, do not move the comment.

	2/4: No changes.

3/4: Update the comment following the explanations from
Eric.

Eric pointed that get_proc_task() without rcu lock can trigger the (bogus) warning. Extract the similar check from pid_delete_dentry() into the new helper and use it instead.

I didn't dare to preserve his ack, but the only change is the new proc_inode_is_dead() helper and

- if (pid_task(proc_pid(inode))
+ if (proc_inode_is_dead(inode))

in proc_task_readdir().

	4/4: New.

Quoted 13 lines · Oleg Nesterov

1. proc_task_readdir() truncates f_pos to long, this can lead to wrong result on 32bit.

2. first_tid() truncates f_pos to int, this is wrong even on 64bit.

We could check that f_pos < PID_MAX or even INT_MAX in proc_task_readdir(), but this patch simply checks the potential overflow in first_tid(), this check is nop on 64bit. We do not care if it was negative and the new unsigned value is huge, all we need to ensure is that we never wrongly return !NULL.

Quoted 3 lines

3. Remove the 2nd "nr != 0" check before get_nr_threads(), nr_threads == 0 is not distinguishable from !pid_task() above.

Oleg, please take a look at the series in vfs.git#experimental; at the very least, we don't want to access file->f_pos in any foo_readdir() - it's too messy and race-prone. It's pretty much independent from the issues you are dealing with, but let's avoid creating pointless conflicts...

Quoted 2 lines

There is another bug in here as well that we may return really crazy things in the case of seek simultaneous with readdir.

i_mutex? both vfs_readdir() and default_llseek() take it... Anyway, this is another issue.

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines

There is another bug in here as well that we may return really crazy things in the case of seek simultaneous with readdir.

i_mutex? both vfs_readdir() and default_llseek() take it... Anyway, this is another issue.

That's part of the reasons for the series I've mentioned (vfs.git#experimental); seriously, check that up.

Will do... but so far I am confused.

I do not see how they could race (I mean /proc/pid/task only). OK, OK, the usage of ->f_pos in sys_getdents() looks "obviously wrong", but this is another story? And "put f_pos in a local variable" can't help.