[PATCH] task name handling in proc fs

[PATCH] task name handling in proc fs

[Mike Kravetz]

We've seen the the top command segfault when dealing with a badly (very
badly) formed task name obtained from a 'stat' file in proc fs.  Upon
further examination, it appears that the task name could be updated at
the same time it is handed off to sprintf in proc_pid_stat().  Now one
could argue that top should be more intelligent and deal with these
badly formed names.  However, I think it's bad to be passing strings that
could be changing to sprintf within the kernel.  I'm pretty sure sprintf
expects the character strings to be static.  Below is a patch to address
this code and one other place dealing with task names in the same file.

-- 
Mike


diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest/fs/proc/array.c	Thu Jul  1 17:44:14 2004
@@ -97,14 +97,14 @@
 		name++;
 		i--;
 		*buf = c;
-		if (!c)
+		if (!*buf)
 			break;
-		if (c == '\\') {
-			buf[1] = c;
+		if (*buf == '\\') {
+			buf[1] = *buf;
 			buf += 2;
 			continue;
 		}
-		if (c == '\n') {
+		if (*buf == '\n') {
 			buf[0] = '\\';
 			buf[1] = 'n';
 			buf += 2;
@@ -308,14 +308,11 @@
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;
+	char tname[sizeof(task->comm)];
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	task_lock(task);
-	mm = task->mm;
-	if(mm)
-		mm = mmgrab(mm);
-	task_unlock(task);
+	mm = get_task_mm(task);
 	if (mm) {
 		down_read(&mm->mmap_sem);
 		vsize = task_vsize(mm);
@@ -357,11 +354,15 @@
 	/* Temporary variable needed for gcc-2.96 */
 	start_time = jiffies_64_to_clock_t(task->start_time - INITIAL_JIFFIES);
 
+	/* Make a static copy of task name for sprintf */
+	memcpy(tname, task->comm, sizeof(tname));
+	tname[sizeof(tname)-1] = '\0';
+
 	res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
 		task->pid,
-		task->comm,
+		tname,
 		state,
 		ppid,
 		pgid,

Re: [PATCH] task name handling in proc fs

[Andrew Morton]

Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> --- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
> +++ linux-2.6.7.ptest/fs/proc/array.c	Thu Jul  1 17:44:14 2004
> @@ -97,14 +97,14 @@
>  		name++;
>  		i--;
>  		*buf = c;
> -		if (!c)
> +		if (!*buf)
>  			break;
> -		if (c == '\\') {
> -			buf[1] = c;
> +		if (*buf == '\\') {
> +			buf[1] = *buf;
>  			buf += 2;
>  			continue;
>  		}
> -		if (c == '\n') {
> +		if (*buf == '\n') {
>  			buf[0] = '\\';
>  			buf[1] = 'n';
>  			buf += 2;

What is this code for?

Re: [PATCH] task name handling in proc fs

[Mike Kravetz]

On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
> Mike Kravetz <kravetz@us.ibm.com> wrote:
> >
> > --- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
> > +++ linux-2.6.7.ptest/fs/proc/array.c	Thu Jul  1 17:44:14 2004
> > @@ -97,14 +97,14 @@
> >  		name++;
> >  		i--;
> >  		*buf = c;
> > -		if (!c)
> > +		if (!*buf)
> >  			break;
> > -		if (c == '\\') {
> > -			buf[1] = c;
> > +		if (*buf == '\\') {
> > +			buf[1] = *buf;
> >  			buf += 2;
> >  			continue;
> >  		}
> > -		if (c == '\n') {
> > +		if (*buf == '\n') {
> >  			buf[0] = '\\';
> >  			buf[1] = 'n';
> >  			buf += 2;
> 
> What is this code for?

The code is copying the task name from 'c' to 'buf' one character
at a time.  It is then 'post processing' the characters.  Currently,
the post processing is based on the value of c which is part of the
source string (task->curr).  However, it is possible for the source
string to change during this copy (think exec).  In such a case I
think it is better to base the 'post processing' on the character
that already has been safely been copied to the target string rather
than the character in the source string which might have changed.

-- 
Mike

Re: [PATCH] task name handling in proc fs

[Andrew Morton]

Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
> > Mike Kravetz <kravetz@us.ibm.com> wrote:
> > >
> > > --- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
> > > +++ linux-2.6.7.ptest/fs/proc/array.c	Thu Jul  1 17:44:14 2004
> > > @@ -97,14 +97,14 @@
> > >  		name++;
> > >  		i--;
> > >  		*buf = c;
> > > -		if (!c)
> > > +		if (!*buf)
> > >  			break;
> > > -		if (c == '\\') {
> > > -			buf[1] = c;
> > > +		if (*buf == '\\') {
> > > +			buf[1] = *buf;
> > >  			buf += 2;
> > >  			continue;
> > >  		}
> > > -		if (c == '\n') {
> > > +		if (*buf == '\n') {
> > >  			buf[0] = '\\';
> > >  			buf[1] = 'n';
> > >  			buf += 2;
> > 
> > What is this code for?
> 
> The code is copying the task name from 'c' to 'buf' one character
> at a time.  It is then 'post processing' the characters.  Currently,
> the post processing is based on the value of c which is part of the
> source string (task->curr).  However, it is possible for the source
> string to change during this copy (think exec).  In such a case I
> think it is better to base the 'post processing' on the character
> that already has been safely been copied to the target string rather
> than the character in the source string which might have changed.

But this just makes it a bit less racy than it used to be.

If we need locking around task->comm (and it seems that we do) then
let's do it.

void get_task_comm(char *buf, struct task_struct *tsk);
void set_task_comm(struct task_struct *tsk, char *buf);

It would be a bit lame to add a new lock for this - probably
task_lock() could be coopted.

Re: [PATCH] task name handling in proc fs

[Mike Kravetz]

On Thu, Jul 01, 2004 at 04:03:35PM -0700, Andrew Morton wrote:
> 
> But this just makes it a bit less racy than it used to be.

Agreed!

> If we need locking around task->comm (and it seems that we do) then
> let's do it.

I'm not sure if there is a need/desire for locking.  But, I don't
have any proc fs history.  What we saw were really badly formed
task names: nulls embedded within strings.  My goal was to simply
provide well formed strings (even if they didn't make sense).

I guess the question is 'how important is it to make sure that
data in these files is consistent/accurate when reported?'.  If it
is highly important, then we need to consider locking down the
task for the duration of gathering all data we want to display
(not just task->comm).  However, my guess is that we would want
to sacrifice this accuracy/consistency to avoid taking locks if
possible.  Again, this is just my opinion and I don't have any
history here ... but would be happy to code up any recommendation.

P.S. Note that these code paths already get the task lock once
as a result of calling get_task_mm().

-- 
Mike

Re: [PATCH] task name handling in proc fs

[Mike Kravetz]

On Thu, Jul 01, 2004 at 04:03:35PM -0700, Andrew Morton wrote:
> 
> If we need locking around task->comm (and it seems that we do) then
> let's do it.
> 
> void get_task_comm(char *buf, struct task_struct *tsk);
> void set_task_comm(struct task_struct *tsk, char *buf);
> 
> It would be a bit lame to add a new lock for this - probably
> task_lock() could be coopted.

OK - Here is a patch to implement/use the routines above.  I couldn't
decide on a good location (file) for them so I put them in 'fs/exec/c'
which is where the existing set_task_comm functionality resides.  In
addition, I took the liberty of replacing the code in 'fs/proc/array.c'
that replicates the code in get_task_mm() with a simple call to the
routine.

-- 
Mike

diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c	Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c	Wed Jul  7 21:46:20 2004
@@ -786,10 +786,33 @@
 	spin_unlock(&files->file_lock);
 }
 
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+	/* buf must be at least sizeof(tsk->comm) in size */
+	task_lock(tsk);
+	memcpy(buf, tsk->comm, sizeof(tsk->comm));
+	task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *name)
+{
+	int i, ch;
+
+	task_lock(tsk);
+	for (i=0; (ch = *(name++)) != '\0';) {
+		if (ch == '/')
+			i = 0;
+		else
+			if (i < (sizeof(tsk->comm) - 1))
+				tsk->comm[i++] = ch;
+	}
+	tsk->comm[i] = '\0';
+	task_unlock(tsk);
+}
+
 int flush_old_exec(struct linux_binprm * bprm)
 {
-	char * name;
-	int i, ch, retval;
+	int retval;
 	struct files_struct *files;
 
 	/*
@@ -826,15 +849,8 @@
 
 	if (current->euid == current->uid && current->egid == current->gid)
 		current->mm->dumpable = 1;
-	name = bprm->filename;
-	for (i=0; (ch = *(name++)) != '\0';) {
-		if (ch == '/')
-			i = 0;
-		else
-			if (i < 15)
-				current->comm[i++] = ch;
-	}
-	current->comm[i] = '\0';
+
+	set_task_comm(current, bprm->filename);
 
 	flush_thread();
 
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c	Wed Jul  7 17:41:28 2004
@@ -88,10 +88,13 @@
 {
 	int i;
 	char * name;
+	char tcomm[sizeof(p->comm)];
+
+	get_task_comm(tcomm, p);
 
 	ADDBUF(buf, "Name:\t");
-	name = p->comm;
-	i = sizeof(p->comm);
+	name = tcomm;
+	i = sizeof(tcomm);
 	do {
 		unsigned char c = *name;
 		name++;
@@ -308,14 +311,11 @@
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;
+	char tcomm[sizeof(task->comm)];
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	task_lock(task);
-	mm = task->mm;
-	if(mm)
-		mm = mmgrab(mm);
-	task_unlock(task);
+	mm = get_task_mm(task);
 	if (mm) {
 		down_read(&mm->mmap_sem);
 		vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
 		up_read(&mm->mmap_sem);
 	}
 
+	get_task_comm(tcomm, task);
 	wchan = get_wchan(task);
 
 	sigemptyset(&sigign);
@@ -361,7 +362,7 @@
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
 		task->pid,
-		task->comm,
+		tcomm,
 		state,
 		ppid,
 		pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h	Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h	Tue Jul  6 22:39:25 2004
@@ -868,6 +868,9 @@
 extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
 #ifdef CONFIG_SMP
 extern void wait_task_inactive(task_t * p);
 #else

Re: [PATCH] task name handling in proc fs

[Andrew Morton]

Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> +void set_task_comm(struct task_struct *tsk, char *name)
> +{
> +	int i, ch;
> +
> +	task_lock(tsk);
> +	for (i=0; (ch = *(name++)) != '\0';) {
> +		if (ch == '/')
> +			i = 0;
> +		else
> +			if (i < (sizeof(tsk->comm) - 1))
> +				tsk->comm[i++] = ch;
> +	}
> +	tsk->comm[i] = '\0';
> +	task_unlock(tsk);
> +}

I don't think the basename logic should be in this function.  Only one
caller needs it, and if we later try to use this function to set
current->comm for per-cpu kernel threads, it will mangle the text.

root         2  0.0  0.0     0    0 ?        SW   Jul06   0:00 [migration/0]
root         3  0.0  0.0     0    0 ?        SWN  Jul06   0:00 [ksoftirqd/0]
root         4  0.0  0.0     0    0 ?        SW   Jul06   0:00 [migration/1]
root         5  0.0  0.0     0    0 ?        SWN  Jul06   0:00 [ksoftirqd/1]
root         6  0.0  0.0     0    0 ?        SW   Jul06   0:00 [migration/2]
root         7  0.0  0.0     0    0 ?        SWN  Jul06   0:00 [ksoftirqd/2]
root         8  0.0  0.0     0    0 ?        SW   Jul06   0:00 [migration/3]
root         9  0.0  0.0     0    0 ?        SWN  Jul06   0:00 [ksoftirqd/3]

We probably won't actually _do_ that, since kthread_create() uses vsnprintf(),
but pushing code which is specific to one caller into a library function
seems wrong...

Re: [PATCH] task name handling in proc fs

[Mike Kravetz]

On Wed, Jul 07, 2004 at 03:11:34PM -0700, Andrew Morton wrote:
> 
> I don't think the basename logic should be in this function.
> 

OK - Here is another version.

-- 
Mike


diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c	Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c	Wed Jul  7 23:30:47 2004
@@ -786,11 +786,35 @@
 	spin_unlock(&files->file_lock);
 }
 
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+	/* buf must be at least sizeof(tsk->comm) in size */
+	task_lock(tsk);
+	memcpy(buf, tsk->comm, sizeof(tsk->comm));
+	task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+	/* buf must be null terminated and <= sizeof(tsk->comm) */
+	int i;
+
+	task_lock(tsk);
+	for(i=0; i<sizeof(tsk->comm); i++) {
+		tsk->comm[i] = *buf++;
+		if (!tsk->comm[i])
+			break;
+	}
+	tsk->comm[sizeof(tsk->comm)-1] = '\0';	/* just in case */
+	task_unlock(tsk);
+}
+
 int flush_old_exec(struct linux_binprm * bprm)
 {
 	char * name;
 	int i, ch, retval;
 	struct files_struct *files;
+	char tcomm[sizeof(current->comm)];
 
 	/*
 	 * Make sure we have a private signal table and that
@@ -826,15 +850,18 @@
 
 	if (current->euid == current->uid && current->egid == current->gid)
 		current->mm->dumpable = 1;
+
 	name = bprm->filename;
 	for (i=0; (ch = *(name++)) != '\0';) {
 		if (ch == '/')
 			i = 0;
 		else
-			if (i < 15)
-				current->comm[i++] = ch;
+			if (i < (sizeof(tcomm) - 1))
+				tcomm[i++] = ch;
 	}
-	current->comm[i] = '\0';
+	tcomm[i] = '\0';
+
+	set_task_comm(current, tcomm);
 
 	flush_thread();
 
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c	Wed Jul  7 17:41:28 2004
@@ -88,10 +88,13 @@
 {
 	int i;
 	char * name;
+	char tcomm[sizeof(p->comm)];
+
+	get_task_comm(tcomm, p);
 
 	ADDBUF(buf, "Name:\t");
-	name = p->comm;
-	i = sizeof(p->comm);
+	name = tcomm;
+	i = sizeof(tcomm);
 	do {
 		unsigned char c = *name;
 		name++;
@@ -308,14 +311,11 @@
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;
+	char tcomm[sizeof(task->comm)];
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	task_lock(task);
-	mm = task->mm;
-	if(mm)
-		mm = mmgrab(mm);
-	task_unlock(task);
+	mm = get_task_mm(task);
 	if (mm) {
 		down_read(&mm->mmap_sem);
 		vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
 		up_read(&mm->mmap_sem);
 	}
 
+	get_task_comm(tcomm, task);
 	wchan = get_wchan(task);
 
 	sigemptyset(&sigign);
@@ -361,7 +362,7 @@
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
 		task->pid,
-		task->comm,
+		tcomm,
 		state,
 		ppid,
 		pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h	Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h	Tue Jul  6 22:39:25 2004
@@ -868,6 +868,9 @@
 extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
 #ifdef CONFIG_SMP
 extern void wait_task_inactive(task_t * p);
 #else

Re: [PATCH] task name handling in proc fs

[Paul Jackson]

+void set_task_comm(struct task_struct *tsk, char *buf)
+{ ...
+	for(i=0; i<sizeof(tsk->comm); i++) {
+		tsk->comm[i] = *buf++;
+		if (!tsk->comm[i])
+			break;
+	}
+	tsk->comm[sizeof(tsk->comm)-1] = '\0';	/* just in case */

That code fragment looks to me like:

	strlcpy (tsk->comm, buf, sizeof(tsk->comm));

Unless I'm missing something, I'd prefer 'strlcpy'.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] task name handling in proc fs

[Mike Kravetz]

On Wed, Jul 07, 2004 at 07:32:22PM -0700, Paul Jackson wrote:
> 
> Unless I'm missing something, I'd prefer 'strlcpy'.
> 

Thanks Paul.  strlcpy isn't one of my commonly used routines, so
it's use didn't occur to me.  Here is the patch using strlcpy.

-- 
Mike


diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c	Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c	Thu Jul  8 16:57:36 2004
@@ -786,11 +786,27 @@
 	spin_unlock(&files->file_lock);
 }
 
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+	/* buf must be at least sizeof(tsk->comm) in size */
+	task_lock(tsk);
+	memcpy(buf, tsk->comm, sizeof(tsk->comm));
+	task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+	task_lock(tsk);
+	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+	task_unlock(tsk);
+}
+
 int flush_old_exec(struct linux_binprm * bprm)
 {
 	char * name;
 	int i, ch, retval;
 	struct files_struct *files;
+	char tcomm[sizeof(current->comm)];
 
 	/*
 	 * Make sure we have a private signal table and that
@@ -826,15 +842,17 @@
 
 	if (current->euid == current->uid && current->egid == current->gid)
 		current->mm->dumpable = 1;
+
 	name = bprm->filename;
 	for (i=0; (ch = *(name++)) != '\0';) {
 		if (ch == '/')
 			i = 0;
 		else
-			if (i < 15)
-				current->comm[i++] = ch;
+			if (i < (sizeof(tcomm) - 1))
+				tcomm[i++] = ch;
 	}
-	current->comm[i] = '\0';
+	tcomm[i] = '\0';
+	set_task_comm(current, tcomm);
 
 	flush_thread();
 
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c	Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c	Wed Jul  7 17:41:28 2004
@@ -88,10 +88,13 @@
 {
 	int i;
 	char * name;
+	char tcomm[sizeof(p->comm)];
+
+	get_task_comm(tcomm, p);
 
 	ADDBUF(buf, "Name:\t");
-	name = p->comm;
-	i = sizeof(p->comm);
+	name = tcomm;
+	i = sizeof(tcomm);
 	do {
 		unsigned char c = *name;
 		name++;
@@ -308,14 +311,11 @@
 	int num_threads = 0;
 	struct mm_struct *mm;
 	unsigned long long start_time;
+	char tcomm[sizeof(task->comm)];
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	task_lock(task);
-	mm = task->mm;
-	if(mm)
-		mm = mmgrab(mm);
-	task_unlock(task);
+	mm = get_task_mm(task);
 	if (mm) {
 		down_read(&mm->mmap_sem);
 		vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
 		up_read(&mm->mmap_sem);
 	}
 
+	get_task_comm(tcomm, task);
 	wchan = get_wchan(task);
 
 	sigemptyset(&sigign);
@@ -361,7 +362,7 @@
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
 %lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
 		task->pid,
-		task->comm,
+		tcomm,
 		state,
 		ppid,
 		pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h	Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h	Tue Jul  6 22:39:25 2004
@@ -868,6 +868,9 @@
 extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
 #ifdef CONFIG_SMP
 extern void wait_task_inactive(task_t * p);
 #else

Re: [PATCH] task name handling in proc fs

[Andrew Rodland]

Mike Kravetz wrote:

> On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
>> Mike Kravetz <kravetz@us.ibm.com> wrote:
>> >
>> > --- linux-2.6.7/fs/proc/array.c    Wed Jun 16 05:19:36 2004
>> > +++ linux-2.6.7.ptest/fs/proc/array.c      Thu Jul  1 17:44:14 2004
>> > @@ -97,14 +97,14 @@
>> >  name++;
>> >  i--;
>> >  *buf = c;
>> > -          if (!c)
>> > +          if (!*buf)
>> >  break;
>> > -          if (c == '\\') {
>> > -                  buf[1] = c;
>> > +          if (*buf == '\\') {
>> > +                  buf[1] = *buf;
>> >  buf += 2;
>> >  continue;
>> >  }
>> > -          if (c == '\n') {
>> > +          if (*buf == '\n') {
>> >  buf[0] = '\\';
>> >  buf[1] = 'n';
>> >  buf += 2;
>> 
>> What is this code for?
> 
> The code is copying the task name from 'c' to 'buf' one character
> at a time.  It is then 'post processing' the characters.  Currently,
> the post processing is based on the value of c which is part of the
> source string (task->curr).  However, it is possible for the source
> string to change during this copy (think exec).

Except that c is not "part of the source string". The code "c =
*name" (where name starts off pointing to the same place as p->comm) is
executed once and only once per time through the loop. Then it does "*buf =
c". Your change would protect not against a change in "name" (which is
possible), but against a change in "buf" while we're writing to it
(impossible, as long as I'm understanding the proc code correctly).

Not that there is no race here, but that doesn't fix it. What's needed is
either another strcpy or locking around p->comm as suggested by Andrew
Morton.

--Andrew Rodland < arodland@entermail.net > via GMANE