do_task_stat: don't use task_pid_nr_ns() lockless

PATCH In review Oleg Nesterov 3 participants 12 messages Nov 17, 2007 – Nov 20, 2007

Subsystems (4)

s390 Proc filesystem Memory mapping Control group - cpuset

lore

Share & tools

lore

Oleg Nesterov

author

2007-11-17 18:31 UTC #0 lore raw

PATCH 1/1 do_task_stat: don't use task_pid_nr_ns() lockless

Without rcu/tasklist/siglock lock task_pid_nr_ns() may read the freed memory, move the callsite under ->siglock.

Sadly, we can report pid == 0 if the task was detached.

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

fs/proc/array.c renamed from fs/proc/array.c~dtst +4 -5

@@ -392,7 +392,7 @@ static int do_task_stat(struct task_stru
 	sigset_t sigign, sigcatch;
 	char state;
 	int res;
-	pid_t ppid = 0, pgid = -1, sid = -1;
+	pid_t pid = 0, ppid = 0, pgid = -1, sid = -1;
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;

@@ -403,9 +403,6 @@ static int do_task_stat(struct task_stru
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
 	unsigned long flags;
-	struct pid_namespace *ns;
-
-	ns = current->nsproxy->pid_ns;
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;

@@ -425,6 +422,7 @@ static int do_task_stat(struct task_stru
 
 	rcu_read_lock();
 	if (lock_task_sighand(task, &flags)) {
+		struct pid_namespace *ns = current->nsproxy->pid_ns;
 		struct signal_struct *sig = task->signal;
 
 		if (sig->tty) {

@@ -461,6 +459,7 @@ static int do_task_stat(struct task_stru
 			gtime = cputime_add(gtime, sig->gtime);
 		}
 
+		pid = task_pid_nr_ns(task, ns);
 		sid = task_session_nr_ns(task, ns);
 		pgid = task_pgrp_nr_ns(task, ns);
 		ppid = task_ppid_nr_ns(task, ns);

@@ -495,7 +494,7 @@ static int do_task_stat(struct task_stru
 	res = sprintf(buffer, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
-		task_pid_nr_ns(task, ns),
+		pid,
 		tcomm,
 		state,
 		ppid,

Eric W. Biederman

2007-11-17 20:12 UTC #1 lore raw

Quoted 4 lines · Oleg Nesterov

Without rcu/tasklist/siglock lock task_pid_nr_ns() may read the freed memory, move the callsite under ->siglock.
Sadly, we can report pid == 0 if the task was detached.

We only get detached in release_task so it is a pretty small window where we can return pid == 0. Usually get_task_pid will fail first and we will return -ESRCH. Still the distance from open to

There is another bug in here as well. current->nsproxy->pid_ns is wrong. What we want is: ns = dentry->d_sb->s_fs_info;

Otherwise we will have file descriptor passing races and the like.

We could also do: proc_pid(inode) to get the pid, which is a little more race free, and will prevent us from returning pid == 0.

In either event it looks like we need to implement some proper file operations for these proc files, maybe even going to seq file status.

Eric

Pavel Emelyanov

2007-11-19 09:31 UTC #2 lore raw

Quoted 5 lines · Eric W. Biederman

Quoted 3 lines · Oleg Nesterov
Without rcu/tasklist/siglock lock task_pid_nr_ns() may read the freed memory, move the callsite under ->siglock.

Quoted 8 lines

Quoted 1 line
Sadly, we can report pid == 0 if the task was detached.
We only get detached in release_task so it is a pretty small window where we can return pid == 0. Usually get_task_pid will fail first and we will return -ESRCH. Still the distance from open to
There is another bug in here as well. current->nsproxy->pid_ns is wrong. What we want is: ns = dentry->d_sb->s_fs_info;

Actually I thought about this recently - if we produce the list of tasks based on the sb's namespace, then we should fill the tasks' files according to the sb's namespace as well, not according to the current namespace.

Quoted 1 line

Otherwise we will have file descriptor passing races and the like.

Can you elaborate?

Quoted 9 lines

We could also do: proc_pid(inode) to get the pid, which is a little more race free, and will prevent us from returning pid == 0.
In either event it looks like we need to implement some proper file operations for these proc files, maybe even going to seq file status.
Eric

Eric W. Biederman

2007-11-19 18:06 UTC #3 lore raw

Quoted 5 lines · Pavel Emelyanov

Quoted 4 lines · Eric W. Biederman
We only get detached in release_task so it is a pretty small window where we can return pid == 0. Usually get_task_pid will fail first and we will return -ESRCH. Still the distance from open to

Quoted 7 lines

Quoted 2 lines
There is another bug in here as well. current->nsproxy->pid_ns is wrong. What we want is: ns = dentry->d_sb->s_fs_info;
Actually I thought about this recently - if we produce the list of tasks based on the sb's namespace, then we should fill the tasks' files according to the sb's namespace as well, not according to the current namespace.

Exactly. That is the problem I was pointing out. Patch in a bit. In do_proc_stat we are currently referencing current->nsproxy->pid_ns;

Quoted 3 lines

Quoted 1 line
Otherwise we will have file descriptor passing races and the like.
Can you elaborate?

Just the classic problem that if you use current at other then open time the contents of the file descriptor may depend on who is reading it which is problematic.

Eric

Pavel Emelyanov

Acked-by on v1 1/1

2007-11-19 09:32 UTC #4 lore raw

In reply to Oleg Nesterov

Quoted 6 lines · Oleg Nesterov

Without rcu/tasklist/siglock lock task_pid_nr_ns() may read the freed memory, move the callsite under ->siglock.
Sadly, we can report pid == 0 if the task was detached.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Pavel Emelyanov <[email protected]>

Quoted 48 lines

fs/proc/array.c renamed from fs/proc/array.c~dtst +4 -5

@@ -392,7 +392,7 @@ static int do_task_stat(struct task_stru
 	sigset_t sigign, sigcatch;
 	char state;
 	int res;
-	pid_t ppid = 0, pgid = -1, sid = -1;
+	pid_t pid = 0, ppid = 0, pgid = -1, sid = -1;
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;

@@ -403,9 +403,6 @@ static int do_task_stat(struct task_stru
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
 	unsigned long flags;
-	struct pid_namespace *ns;
-
-	ns = current->nsproxy->pid_ns;
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;

@@ -425,6 +422,7 @@ static int do_task_stat(struct task_stru
 
 	rcu_read_lock();
 	if (lock_task_sighand(task, &flags)) {
+		struct pid_namespace *ns = current->nsproxy->pid_ns;
 		struct signal_struct *sig = task->signal;
 
 		if (sig->tty) {

@@ -461,6 +459,7 @@ static int do_task_stat(struct task_stru
 			gtime = cputime_add(gtime, sig->gtime);
 		}
 
+		pid = task_pid_nr_ns(task, ns);
 		sid = task_session_nr_ns(task, ns);
 		pgid = task_pgrp_nr_ns(task, ns);
 		ppid = task_ppid_nr_ns(task, ns);

@@ -495,7 +494,7 @@ static int do_task_stat(struct task_stru
 	res = sprintf(buffer, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
-		task_pid_nr_ns(task, ns),
+		pid,
 		tcomm,
 		state,
 		ppid,

Eric W. Biederman

2007-11-19 22:01 UTC #5 lore raw

cover proc: Cleanup status files.

In reply to Oleg Nesterov

There is a long standing ugliness with /proc/<pid>/stat, /proc/<pid>/statm, and /proc/<pid>/status that they do not use the seqfile API.

In addition they are currently reporting pids in the pid namespace of the current task instead of the pid namespace with which proc was mounted which is confusing.

This patch series first build the infrastructure to allow these problems to be fixed and then it converts the individual proc status files.

Eric

Eric W. Biederman

2007-11-19 22:06 UTC #6 lore raw

PATCH 1/4 proc: Implement proc_single_file_operations

Currently many /proc/pid files use a crufty precursor to the current seq_file api, and they don't have direct access to the pid_namespace or the pid of for which they are displaying data.

So implement proc_single_file_operations to make the seq_file routines easy to use, and to give access to the full state of the pid of we are displaying data for.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/proc/base.c          |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/proc_fs.h |    3 +++
 2 files changed, 46 insertions(+), 0 deletions(-)

fs/proc/base.c +43 -0

index a17c268..e9ff77e 100644
@@ -125,6 +125,10 @@ struct pid_entry {
 	NOD(NAME, (S_IFREG|(MODE)), 			\
 		NULL, &proc_info_file_operations,	\
 		{ .proc_read = &proc_##OTYPE } )
+#define ONE(NAME, MODE, OTYPE)				\
+	NOD(NAME, (S_IFREG|(MODE)), 			\
+		NULL, &proc_single_file_operations,	\
+		{ .proc_show = &proc_##OTYPE } )
 
 int maps_protect;
 EXPORT_SYMBOL(maps_protect);

@@ -571,6 +575,45 @@ static const struct file_operations proc_info_file_operations = {
 	.read		= proc_info_read,
 };
 
+static int proc_single_show(struct seq_file *m, void *v)
+{
+	struct inode *inode = m->private;
+	struct pid_namespace *ns;
+	struct pid *pid;
+	struct task_struct *task;
+	int ret;
+
+	ns = inode->i_sb->s_fs_info;
+	pid = proc_pid(inode);
+	task = get_pid_task(pid, PIDTYPE_PID);
+	if (!task)
+		return -ESRCH;
+
+	ret = PROC_I(inode)->op.proc_show(m, ns, pid, task);
+
+	put_task_struct(task);
+	return ret;
+}
+
+static int proc_single_open(struct inode *inode, struct file *filp)
+{
+	int ret;
+	ret = single_open(filp, proc_single_show, NULL);
+	if (!ret) {
+		struct seq_file *m = filp->private_data;
+
+		m->private = inode;
+	}
+	return ret;
+}
+
+static const struct file_operations proc_single_file_operations = {
+	.open		= proc_single_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int mem_open(struct inode* inode, struct file* file)
 {
 	file->private_data = (void*)((long)current->self_exec_id);

include/linux/proc_fs.h +3 -0

index 1273c6e..69307c9 100644
@@ -257,6 +257,9 @@ extern void kclist_add(struct kcore_list *, void *, size_t);
 union proc_op {
 	int (*proc_get_link)(struct inode *, struct dentry **, struct vfsmount **);
 	int (*proc_read)(struct task_struct *task, char *page);
+	int (*proc_show)(struct seq_file *m,
+		struct pid_namespace *ns, struct pid *pid,
+		struct task_struct *task);
 };
 
 struct proc_inode {

Signature · 1 line

1.5.3.rc6.17.g1911

Eric W. Biederman

2007-11-19 22:10 UTC #7 lore raw

PATCH 2/4 proc: Rewrite do_task_stat to correctly handle pid namespaces.

Currently (as pointed out by Oleg) do_task_stat has a race when calling task_pid_nr_ns with the task exiting. In addition do_task_stat is not currently displaying information in the context of the pid namespace that mounted the /proc filesystem. So "cut -d' ' -f 1 /proc/<pid>/stat" may not equal <pid>.

This patch fixes the problem by converting to a single_open seq_file show method. Getting the pid namespace from the filesystem superblock instead of current, and simply using the the struct pid from the inode instead of attempting to get that same pid from the task.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/proc/array.c    |   24 ++++++++++++------------
 fs/proc/base.c     |    4 ++--
 fs/proc/internal.h |    4 ++--
 3 files changed, 16 insertions(+), 16 deletions(-)

fs/proc/array.c +12 ~~-12~~

index eba339e..e7d290f 100644
@@ -77,6 +77,7 @@
 #include <linux/cpuset.h>
 #include <linux/rcupdate.h>
 #include <linux/delayacct.h>
+#include <linux/seq_file.h>
 #include <linux/pid_namespace.h>
 
 #include <asm/pgtable.h>

@@ -384,14 +385,14 @@ static cputime_t task_gtime(struct task_struct *p)
 	return p->gtime;
 }
 
-static int do_task_stat(struct task_struct *task, char *buffer, int whole)
+static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
+			struct pid *pid, struct task_struct *task, int whole)
 {
 	unsigned long vsize, eip, esp, wchan = ~0UL;
 	long priority, nice;
 	int tty_pgrp = -1, tty_nr = 0;
 	sigset_t sigign, sigcatch;
 	char state;
-	int res;
 	pid_t ppid = 0, pgid = -1, sid = -1;
 	int num_threads = 0;
 	struct mm_struct *mm;

@@ -403,9 +404,6 @@ static int do_task_stat(struct task_struct *task, char *buffer, int whole)
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
 	unsigned long flags;
-	struct pid_namespace *ns;
-
-	ns = current->nsproxy->pid_ns;
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;

@@ -492,10 +490,10 @@ static int do_task_stat(struct task_struct *task, char *buffer, int whole)
 	/* convert nsec -> ticks */
 	start_time = nsec_to_clock_t(start_time);
 
-	res = sprintf(buffer, "%d (%s) %c %d %d %d %d %d %u %lu \
+	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
-		task_pid_nr_ns(task, ns),
+		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
 		ppid,

@@ -544,17 +542,19 @@ static int do_task_stat(struct task_struct *task, char *buffer, int whole)
 		cputime_to_clock_t(cgtime));
 	if (mm)
 		mmput(mm);
-	return res;
+	return 0;
 }
 
-int proc_tid_stat(struct task_struct *task, char *buffer)
+int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns,
+			struct pid *pid, struct task_struct *task)
 {
-	return do_task_stat(task, buffer, 0);
+	return do_task_stat(m, ns, pid, task, 0);
 }
 
-int proc_tgid_stat(struct task_struct *task, char *buffer)
+int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns,
+			struct pid *pid, struct task_struct *task)
 {
-	return do_task_stat(task, buffer, 1);
+	return do_task_stat(m, ns, pid, task, 1);
 }
 
 int proc_pid_statm(struct task_struct *task, char *buffer)

fs/proc/base.c +2 -2

index e9ff77e..51f0e7a 100644
@@ -2228,7 +2228,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("sched",      S_IRUGO|S_IWUSR, pid_sched),
 #endif
 	INF("cmdline",    S_IRUGO, pid_cmdline),
-	INF("stat",       S_IRUGO, tgid_stat),
+	ONE("stat",       S_IRUGO, tgid_stat),
 	INF("statm",      S_IRUGO, pid_statm),
 	REG("maps",       S_IRUGO, maps),
 #ifdef CONFIG_NUMA

@@ -2549,7 +2549,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("sched",     S_IRUGO|S_IWUSR, pid_sched),
 #endif
 	INF("cmdline",   S_IRUGO, pid_cmdline),
-	INF("stat",      S_IRUGO, tid_stat),
+	ONE("stat",      S_IRUGO, tid_stat),
 	INF("statm",     S_IRUGO, pid_statm),
 	REG("maps",      S_IRUGO, maps),
 #ifdef CONFIG_NUMA

fs/proc/internal.h +2 -2

index 1b2b6c6..74bc772 100644
@@ -46,8 +46,8 @@ extern int maps_protect;
 
 extern void create_seq_entry(char *name, mode_t mode, const struct file_operations *f);
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
-extern int proc_tid_stat(struct task_struct *,  char *);
-extern int proc_tgid_stat(struct task_struct *, char *);
+extern int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
+extern int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 extern int proc_pid_status(struct task_struct *, char *);
 extern int proc_pid_statm(struct task_struct *, char *);

Signature · 1 line

1.5.3.rc6.17.g1911

Eric W. Biederman

2007-11-19 22:12 UTC #8 lore raw

PATCH 3/4 proc: seqfile convert proc_pid_status to properly handle pid namespaces

Currently we possibly lookup the pid in the wrong pid namespace. So seq_file convert proc_pid_status which ensures the proper pid namespaces is passed in, and ensures that we don't overflow any internal buffers when generating /proc/<pid>/status

Signed-off-by Eric W. Biederman <[email protected]>

---
 arch/s390/kernel/traps.c     |   23 ++++----
 fs/proc/array.c              |  122 ++++++++++++++++++++----------------------
 fs/proc/base.c               |    4 +-
 fs/proc/internal.h           |    2 +-
 fs/proc/task_mmu.c           |    6 +-
 fs/proc/task_nommu.c         |    2 +-
 include/asm-s390/processor.h |    2 +-
 include/linux/cpuset.h       |    8 ++--
 include/linux/proc_fs.h      |    2 +-
 kernel/cpuset.c              |   18 ++++---
 10 files changed, 93 insertions(+), 96 deletions(-)

arch/s390/kernel/traps.c +11 ~~-12~~

index 8ec9def..fd75106 100644
@@ -210,41 +210,40 @@ void show_registers(struct pt_regs *regs)
 }	
 
 /* This is called from fs/proc/array.c */
-char *task_show_regs(struct task_struct *task, char *buffer)
+void task_show_regs(struct seq_file *m, struct task_struct *task)
 {
 	struct pt_regs *regs;
 
 	regs = task_pt_regs(task);
-	buffer += sprintf(buffer, "task: %p, ksp: %p\n",
+	seq_printf(m, "task: %p, ksp: %p\n",
 		       task, (void *)task->thread.ksp);
-	buffer += sprintf(buffer, "User PSW : %p %p\n",
+	seq_printf(m, "User PSW : %p %p\n",
 		       (void *) regs->psw.mask, (void *)regs->psw.addr);
 
-	buffer += sprintf(buffer, "User GPRS: " FOURLONG,
+	seq_printf(m, "User GPRS: " FOURLONG,
 			  regs->gprs[0], regs->gprs[1],
 			  regs->gprs[2], regs->gprs[3]);
-	buffer += sprintf(buffer, "           " FOURLONG,
+	seq_printf(m, "           " FOURLONG,
 			  regs->gprs[4], regs->gprs[5],
 			  regs->gprs[6], regs->gprs[7]);
-	buffer += sprintf(buffer, "           " FOURLONG,
+	seq_printf(m, "           " FOURLONG,
 			  regs->gprs[8], regs->gprs[9],
 			  regs->gprs[10], regs->gprs[11]);
-	buffer += sprintf(buffer, "           " FOURLONG,
+	seq_printf(m, "           " FOURLONG,
 			  regs->gprs[12], regs->gprs[13],
 			  regs->gprs[14], regs->gprs[15]);
-	buffer += sprintf(buffer, "User ACRS: %08x %08x %08x %08x\n",
+	seq_printf(m, "User ACRS: %08x %08x %08x %08x\n",
 			  task->thread.acrs[0], task->thread.acrs[1],
 			  task->thread.acrs[2], task->thread.acrs[3]);
-	buffer += sprintf(buffer, "           %08x %08x %08x %08x\n",
+	seq_printf(m, "           %08x %08x %08x %08x\n",
 			  task->thread.acrs[4], task->thread.acrs[5],
 			  task->thread.acrs[6], task->thread.acrs[7]);
-	buffer += sprintf(buffer, "           %08x %08x %08x %08x\n",
+	seq_printf(m, "           %08x %08x %08x %08x\n",
 			  task->thread.acrs[8], task->thread.acrs[9],
 			  task->thread.acrs[10], task->thread.acrs[11]);
-	buffer += sprintf(buffer, "           %08x %08x %08x %08x\n",
+	seq_printf(m, "           %08x %08x %08x %08x\n",
 			  task->thread.acrs[12], task->thread.acrs[13],
 			  task->thread.acrs[14], task->thread.acrs[15]);
-	return buffer;
 }
 
 static DEFINE_SPINLOCK(die_lock);

fs/proc/array.c +59 ~~-63~~

229 lines not on this page · raw message

fs/proc/base.c +2 -2

index 51f0e7a..086a262 100644
@@ -2222,7 +2222,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	DIR("fdinfo",     S_IRUSR|S_IXUSR, fdinfo),
 	REG("environ",    S_IRUSR, environ),
 	INF("auxv",       S_IRUSR, pid_auxv),
-	INF("status",     S_IRUGO, pid_status),
+	ONE("status",     S_IRUGO, pid_status),
 	INF("limits",	  S_IRUSR, pid_limits),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",      S_IRUGO|S_IWUSR, pid_sched),

@@ -2543,7 +2543,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	DIR("fdinfo",    S_IRUSR|S_IXUSR, fdinfo),
 	REG("environ",   S_IRUSR, environ),
 	INF("auxv",      S_IRUSR, pid_auxv),
-	INF("status",    S_IRUGO, pid_status),
+	ONE("status",    S_IRUGO, pid_status),
 	INF("limits",	 S_IRUSR, pid_limits),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",     S_IRUGO|S_IWUSR, pid_sched),

fs/proc/internal.h +1 -1

index 74bc772..0033645 100644
@@ -48,7 +48,7 @@ extern void create_seq_entry(char *name, mode_t mode, const struct file_operatio
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 extern int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
-extern int proc_pid_status(struct task_struct *, char *);
+extern int proc_pid_status(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 extern int proc_pid_statm(struct task_struct *, char *);
 
 extern const struct file_operations proc_maps_operations;

fs/proc/task_mmu.c +3 -3

index c24d81a..d5cb8fb 100644
@@ -6,13 +6,14 @@
 #include <linux/ptrace.h>
 #include <linux/pagemap.h>
 #include <linux/mempolicy.h>
+#include <linux/seq_file.h>
 
 #include <asm/elf.h>
 #include <asm/uaccess.h>
 #include <asm/tlbflush.h>
 #include "internal.h"
 
-char *task_mem(struct mm_struct *mm, char *buffer)
+void task_mem(struct seq_file *m, struct mm_struct *mm)
 {
 	unsigned long data, text, lib;
 	unsigned long hiwater_vm, total_vm, hiwater_rss, total_rss;

@@ -34,7 +35,7 @@ char *task_mem(struct mm_struct *mm, char *buffer)
 	data = mm->total_vm - mm->shared_vm - mm->stack_vm;
 	text = (PAGE_ALIGN(mm->end_code) - (mm->start_code & PAGE_MASK)) >> 10;
 	lib = (mm->exec_vm << (PAGE_SHIFT-10)) - text;
-	buffer += sprintf(buffer,
+	seq_printf(m, 
 		"VmPeak:\t%8lu kB\n"
 		"VmSize:\t%8lu kB\n"
 		"VmLck:\t%8lu kB\n"

@@ -53,7 +54,6 @@ char *task_mem(struct mm_struct *mm, char *buffer)
 		data << (PAGE_SHIFT-10),
 		mm->stack_vm << (PAGE_SHIFT-10), text, lib,
 		(PTRS_PER_PTE*sizeof(pte_t)*mm->nr_ptes) >> 10);
-	return buffer;
 }
 
 unsigned long task_vsize(struct mm_struct *mm)

fs/proc/task_nommu.c +1 -1

index d8b8c71..d4e9495 100644
@@ -12,7 +12,7 @@
  * each process that owns it. Non-shared memory is counted
  * accurately.
  */
-char *task_mem(struct mm_struct *mm, char *buffer)
+void task_mem(struct seq_file *m, struct mm_struct *mm)
 {
 	struct vm_list_struct *vml;
 	unsigned long bytes = 0, sbytes = 0, slack = 0;

include/asm-s390/processor.h +1 -1

index 21d40a1..3d4ff06 100644
@@ -180,7 +180,7 @@ extern unsigned long thread_saved_pc(struct task_struct *t);
 /*
  * Print register of task into buffer. Used in fs/proc/array.c.
  */
-extern char *task_show_regs(struct task_struct *task, char *buffer);
+extern void task_show_regs(struct seq_file *m, struct task_struct *task);
 
 extern void show_registers(struct pt_regs *regs);
 extern void show_code(struct pt_regs *regs);

include/linux/cpuset.h +4 -4

index ecae585..3eb2e81 100644
@@ -57,7 +57,8 @@ extern int cpuset_memory_pressure_enabled;
 extern void __cpuset_memory_pressure_bump(void);
 
 extern const struct file_operations proc_cpuset_operations;
-extern char *cpuset_task_status_allowed(struct task_struct *task, char *buffer);
+struct seq_file;
+extern void cpuset_task_status_allowed(struct seq_file *m, struct task_struct *task);
 
 extern void cpuset_lock(void);
 extern void cpuset_unlock(void);

@@ -126,10 +127,9 @@ static inline int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
 
 static inline void cpuset_memory_pressure_bump(void) {}
 
-static inline char *cpuset_task_status_allowed(struct task_struct *task,
-							char *buffer)
+static inline void cpuset_task_status_allowed(struct seq_file *m, 
+						struct task_struct *task)
 {
-	return buffer;
 }
 
 static inline void cpuset_lock(void) {}

include/linux/proc_fs.h +1 -1

index 69307c9..b070b3b 100644
@@ -114,7 +114,7 @@ struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, struct
 int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir);
 unsigned long task_vsize(struct mm_struct *);
 int task_statm(struct mm_struct *, int *, int *, int *, int *);
-char *task_mem(struct mm_struct *, char *);
+void task_mem(struct seq_file *, struct mm_struct *);
 void clear_refs_smap(struct mm_struct *mm);
 
 struct proc_dir_entry *de_get(struct proc_dir_entry *de);

kernel/cpuset.c +10 -8

index 50f5dc4..9a0ebcf 100644
@@ -2219,13 +2219,15 @@ const struct file_operations proc_cpuset_operations = {
 #endif /* CONFIG_PROC_PID_CPUSET */
 
 /* Display task cpus_allowed, mems_allowed in /proc/<pid>/status file. */
-char *cpuset_task_status_allowed(struct task_struct *task, char *buffer)
+void cpuset_task_status_allowed(struct seq_file *m, struct task_struct *task)
 {
-	buffer += sprintf(buffer, "Cpus_allowed:\t");
-	buffer += cpumask_scnprintf(buffer, PAGE_SIZE, task->cpus_allowed);
-	buffer += sprintf(buffer, "\n");
-	buffer += sprintf(buffer, "Mems_allowed:\t");
-	buffer += nodemask_scnprintf(buffer, PAGE_SIZE, task->mems_allowed);
-	buffer += sprintf(buffer, "\n");
-	return buffer;
+	int len;
+	seq_printf(m, "Cpus_allowed:\t");
+	m->count += cpumask_scnprintf(m->buf + m->count, m->size - m->count,
+					task->cpus_allowed);
+	seq_printf(m, "\n");
+	seq_printf(m, "Mems_allowed:\t");
+	m->count += nodemask_scnprintf(m->buf + m->count, m->size - m->count,
+					task->mems_allowed);
+	seq_printf(buffer, "\n");
 }

Signature · 1 line

1.5.3.rc6.17.g1911

Eric W. Biederman

2007-11-19 22:14 UTC #9 lore raw

PATCH 1/1 proc: seqfile convert proc_pid_statm

This conversion is just for code cleanliness, uniformity, and general safety.

Signed-off-by Eric W. Biederman <[email protected]>

---
 fs/proc/array.c    |    8 +++++---
 fs/proc/base.c     |    4 ++--
 fs/proc/internal.h |    2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

fs/proc/array.c +5 -3

index d23d91d..b1a90c0 100644
@@ -553,7 +553,8 @@ int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns,
 	return do_task_stat(m, ns, pid, task, 1);
 }
 
-int proc_pid_statm(struct task_struct *task, char *buffer)
+int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
+			struct pid *pid, struct task_struct *task)
 {
 	int size = 0, resident = 0, shared = 0, text = 0, lib = 0, data = 0;
 	struct mm_struct *mm = get_task_mm(task);

@@ -562,7 +563,8 @@ int proc_pid_statm(struct task_struct *task, char *buffer)
 		size = task_statm(mm, &shared, &text, &data, &resident);
 		mmput(mm);
 	}
+	seq_printf(m, "%d %d %d %d %d %d %d\n",
+			size, resident, shared, text, lib, data, 0);
 
-	return sprintf(buffer, "%d %d %d %d %d %d %d\n",
-		       size, resident, shared, text, lib, data, 0);
+	return 0;
 }

fs/proc/base.c +2 -2

index 086a262..5b92b79 100644
@@ -2229,7 +2229,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #endif
 	INF("cmdline",    S_IRUGO, pid_cmdline),
 	ONE("stat",       S_IRUGO, tgid_stat),
-	INF("statm",      S_IRUGO, pid_statm),
+	ONE("statm",      S_IRUGO, pid_statm),
 	REG("maps",       S_IRUGO, maps),
 #ifdef CONFIG_NUMA
 	REG("numa_maps",  S_IRUGO, numa_maps),

@@ -2550,7 +2550,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 	INF("cmdline",   S_IRUGO, pid_cmdline),
 	ONE("stat",      S_IRUGO, tid_stat),
-	INF("statm",     S_IRUGO, pid_statm),
+	ONE("statm",     S_IRUGO, pid_statm),
 	REG("maps",      S_IRUGO, maps),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, numa_maps),

fs/proc/internal.h +1 -1

index 0033645..4c17360 100644
@@ -49,7 +49,7 @@ extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 extern int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 extern int proc_pid_status(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
-extern int proc_pid_statm(struct task_struct *, char *);
+extern int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns, struct pid *pid, struct task_struct *task);
 
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;

Signature · 1 line

1.5.3.rc6.17.g1911

Pavel Emelyanov

Acked-by on the v1 series

2007-11-20 10:35 UTC #10 lore raw

Re: [PATCH 0/4] proc: Cleanup status files.

In reply to Eric W. Biederman

Quoted 7 lines · Eric W. Biederman

There is a long standing ugliness with /proc/<pid>/stat, /proc/<pid>/statm, and /proc/<pid>/status that they do not use the seqfile API.
In addition they are currently reporting pids in the pid namespace of the current task instead of the pid namespace with which proc was mounted which is confusing.

100% agree with that. Thanks for fixing this issue :)

Acked-by Pavel Emelyanov <[email protected]>

Quoted 12 lines

This patch series first build the infrastructure to allow these problems to be fixed and then it converts the individual proc status files.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Eric W. Biederman

2007-11-19 22:31 UTC #11 lore raw

PATCH 1/1 proc: Proper pidns handling for /proc/self

In reply to Oleg Nesterov

Currently if you access a /proc that is not mounted with your processes current pid namespace /proc/self will point at a completely different process. Ouch!.

This patch fixes /proc/self to point to the current process if it is available in the particular mount of /proc or to return -ENOENT if the current process is not visible.

Signed-off-by Eric W. Biederman <[email protected]>

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

fs/proc/base.c +10 -2

index 7e8eca4..34a1821 100644
@@ -2049,15 +2049,23 @@ static const struct file_operations proc_coredump_filter_operations = {
 static int proc_self_readlink(struct dentry *dentry, char __user *buffer,
 			      int buflen)
 {
+	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
+	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char tmp[PROC_NUMBUF];
-	sprintf(tmp, "%d", task_tgid_vnr(current));
+	if (!tgid)
+		return -ENOENT;
+	sprintf(tmp, "%d", tgid);
 	return vfs_readlink(dentry,buffer,buflen,tmp);
 }
 
 static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
+	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
+	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char tmp[PROC_NUMBUF];
-	sprintf(tmp, "%d", task_tgid_vnr(current));
+	if (!tgid)
+		return ERR_PTR(-ENOENT);
+	sprintf(tmp, "%d", task_tgid_nr_ns(current, ns));
 	return ERR_PTR(vfs_follow_link(nd,tmp));
 }

Signature · 1 line

1.5.3.rc6.17.g1911