* [PATCH] cpusets: fix possible race in cpuset_tasks_read()
@ 2004-09-10 14:38 Simon Derr
2004-09-11 8:01 ` [PATCH] cpusets: alternative fix for " Paul Jackson
0 siblings, 1 reply; 5+ messages in thread
From: Simon Derr @ 2004-09-10 14:38 UTC (permalink / raw)
To: pj, akpm, linux-kernel
Hi,
this patch fixes a possible race between two threads of a single process
in cpuset_tasks_read().
It is mostly the same issue as the one that was in sysfs.
This patch is against 2.6.9-rc1-mm4.
Signed-Off-By: Simon Derr <simon.derr@bull.net>
Index: mm4/kernel/cpuset.c
===================================================================
--- mm4.orig/kernel/cpuset.c 2004-09-07 11:36:18.000000000 +0200
+++ mm4/kernel/cpuset.c 2004-09-10 15:10:01.657243216 +0200
@@ -1100,20 +1100,27 @@
static ssize_t cpuset_tasks_read(struct file *file, char __user *buf,
size_t nbytes, loff_t *ppos)
{
- struct ctr_struct *ctr = (struct ctr_struct *)file->private_data;
+ struct ctr_struct *ctr;
+ down(&cpuset_sem);
+ ctr = (struct ctr_struct *)file->private_data;
/* allocate buffer and fill it on first call to read() */
if (!ctr) {
ctr = cpuset_tasks_mkctr(file);
- if (!ctr)
+ if (!ctr) {
+ up(&cpuset_sem);
return -ENOMEM;
+ }
}
if (*ppos + nbytes > ctr->bufsz)
nbytes = ctr->bufsz - *ppos;
- if (copy_to_user(buf, ctr->buf + *ppos, nbytes))
+ if (copy_to_user(buf, ctr->buf + *ppos, nbytes)) {
+ up(&cpuset_sem);
return -EFAULT;
+ }
*ppos += nbytes;
+ up(&cpuset_sem);
return nbytes;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cpusets: alternative fix for possible race in cpuset_tasks_read()
2004-09-10 14:38 [PATCH] cpusets: fix possible race in cpuset_tasks_read() Simon Derr
@ 2004-09-11 8:01 ` Paul Jackson
2004-09-13 8:24 ` Simon.Derr
0 siblings, 1 reply; 5+ messages in thread
From: Paul Jackson @ 2004-09-11 8:01 UTC (permalink / raw)
To: Simon Derr; +Cc: akpm, linux-kernel
Here's an alternative fix for the race condition on read that Simon
reports.
Andrew,
Don't apply this one yet, until Simon Derr gets a chance to
compare with his alternative patch, and render his analysis.
Move the code that sets up the character buffer of text to read out
when reading a "tasks" file from the read routine to the open routine.
Multiple cloned threads could be doing the first read on a shared
file descriptor open on a "tasks" file, resulting in confused
or leaked kernel memory as multiple threads initialized the same
file private_data at the same time. Rather than add locks to the
initialization code, move it into the open(), where it belongs anyway.
Signed-off-by: Paul Jackson
Index: 2.6.9-rc1-mm4/kernel/cpuset.c
===================================================================
--- 2.6.9-rc1-mm4.orig/kernel/cpuset.c 2004-09-10 15:27:32.000000000 -0700
+++ 2.6.9-rc1-mm4/kernel/cpuset.c 2004-09-10 21:20:02.000000000 -0700
@@ -1034,7 +1034,7 @@ static int pid_array_to_buf(char *buf, i
return cnt;
}
-static inline struct ctr_struct *cpuset_tasks_mkctr(struct file *file)
+static int cpuset_tasks_open(struct inode *unused, struct file *file)
{
struct cpuset *cs = __d_cs(file->f_dentry->d_parent);
struct ctr_struct *ctr;
@@ -1069,14 +1069,14 @@ static inline struct ctr_struct *cpuset_
kfree(pidarray);
file->private_data = (void *)ctr;
- return ctr;
+ return 0;
err2:
kfree(pidarray);
err1:
kfree(ctr);
err0:
- return NULL;
+ return -ENOMEM;
}
static ssize_t cpuset_tasks_read(struct file *file, char __user *buf,
@@ -1084,13 +1084,6 @@ static ssize_t cpuset_tasks_read(struct
{
struct ctr_struct *ctr = (struct ctr_struct *)file->private_data;
- /* allocate buffer and fill it on first call to read() */
- if (!ctr) {
- ctr = cpuset_tasks_mkctr(file);
- if (!ctr)
- return -ENOMEM;
- }
-
if (*ppos + nbytes > ctr->bufsz)
nbytes = ctr->bufsz - *ppos;
if (copy_to_user(buf, ctr->buf + *ppos, nbytes))
@@ -1121,6 +1114,7 @@ static int cpuset_tasks_release(struct i
static struct cftype cft_tasks = {
.name = "tasks",
+ .open = cpuset_tasks_open,
.read = cpuset_tasks_read,
.release = cpuset_tasks_release,
.private = FILE_TASKLIST,
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.650.933.1373
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpusets: alternative fix for possible race in cpuset_tasks_read()
2004-09-11 8:01 ` [PATCH] cpusets: alternative fix for " Paul Jackson
@ 2004-09-13 8:24 ` Simon.Derr
2004-09-13 9:25 ` Paul Jackson
0 siblings, 1 reply; 5+ messages in thread
From: Simon.Derr @ 2004-09-13 8:24 UTC (permalink / raw)
To: Paul Jackson; +Cc: Simon Derr, akpm, linux-kernel
On Sat, 11 Sep 2004, Paul Jackson wrote:
> Here's an alternative fix for the race condition on read that Simon
> reports.
>
> Andrew,
>
> Don't apply this one yet, until Simon Derr gets a chance to
> compare with his alternative patch, and render his analysis.
>
> Move the code that sets up the character buffer of text to read out
> when reading a "tasks" file from the read routine to the open routine.
>
> Multiple cloned threads could be doing the first read on a shared
> file descriptor open on a "tasks" file, resulting in confused
> or leaked kernel memory as multiple threads initialized the same
> file private_data at the same time. Rather than add locks to the
> initialization code, move it into the open(), where it belongs anyway.
Indeed, this code belongs to open(). It was in read() for foolish reasons.
Your patch looks good, but I polished it a bit, so we do the buffer
allocation in open() only when the file is opened for reading.
Signed-off-by: Paul Jackson
Signed-off-by: Simon Derr <simon.derr@bull.net>
Index: mm4/kernel/cpuset.c
===================================================================
--- mm4.orig/kernel/cpuset.c 2004-09-07 11:36:18.000000000 +0200
+++ mm4/kernel/cpuset.c 2004-09-13 09:43:02.282327670 +0200
@@ -1052,13 +1052,16 @@ static int pid_array_to_buf(char *buf, i
return cnt;
}
-static inline struct ctr_struct *cpuset_tasks_mkctr(struct file *file)
+static int cpuset_tasks_open(struct inode *unused, struct file *file)
{
struct cpuset *cs = __d_cs(file->f_dentry->d_parent);
struct ctr_struct *ctr;
pid_t *pidarray;
int npids;
char c;
+
+ if (!(file->f_mode & FMODE_READ))
+ return 0;
ctr = kmalloc(sizeof(*ctr), GFP_KERNEL);
if (!ctr)
@@ -1087,14 +1090,14 @@ static inline struct ctr_struct *cpuset_
kfree(pidarray);
file->private_data = (void *)ctr;
- return ctr;
+ return 0;
err2:
kfree(pidarray);
err1:
kfree(ctr);
err0:
- return NULL;
+ return -ENOMEM;
}
static ssize_t cpuset_tasks_read(struct file *file, char __user *buf,
@@ -1102,13 +1105,6 @@ static ssize_t cpuset_tasks_read(struct
{
struct ctr_struct *ctr = (struct ctr_struct *)file->private_data;
- /* allocate buffer and fill it on first call to read() */
- if (!ctr) {
- ctr = cpuset_tasks_mkctr(file);
- if (!ctr)
- return -ENOMEM;
- }
-
if (*ppos + nbytes > ctr->bufsz)
nbytes = ctr->bufsz - *ppos;
if (copy_to_user(buf, ctr->buf + *ppos, nbytes))
@@ -1121,12 +1117,8 @@ static int cpuset_tasks_release(struct i
{
struct ctr_struct *ctr;
- /* we have nothing to do if no read-access is needed */
- if (!(file->f_mode & FMODE_READ))
- return 0;
-
- ctr = (struct ctr_struct *)file->private_data;
- if (ctr) {
+ if (file->f_mode & FMODE_READ) {
+ ctr = (struct ctr_struct *)file->private_data;
kfree(ctr->buf);
kfree(ctr);
}
@@ -1139,6 +1131,7 @@ static int cpuset_tasks_release(struct i
static struct cftype cft_tasks = {
.name = "tasks",
+ .open = cpuset_tasks_open,
.read = cpuset_tasks_read,
.release = cpuset_tasks_release,
.private = FILE_TASKLIST,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpusets: alternative fix for possible race in cpuset_tasks_read()
2004-09-13 8:24 ` Simon.Derr
@ 2004-09-13 9:25 ` Paul Jackson
2004-09-13 9:39 ` Paul Jackson
0 siblings, 1 reply; 5+ messages in thread
From: Paul Jackson @ 2004-09-13 9:25 UTC (permalink / raw)
To: Simon.Derr; +Cc: simon.derr, akpm, linux-kernel
Andrew,
This is ready to go in with the cpuset patches in *-mm. Simon's latest
changes to buffer only when reading are good - thanks, Simon. I fixed up
the diff formatting - Simon's latest patch had botched leading space chars.
==
Move the code that sets up the character buffer of text to read out
when reading a "tasks" file from the read routine to the open routine.
Multiple cloned threads could be doing the first read on a shared
file descriptor open on a "tasks" file, resulting in confused
or leaked kernel memory as multiple threads initialized the same
file private_data at the same time. Rather than add locks to the
initialization code, move it into the open(), where it belongs anyway.
Only do buffer allocation in open() when the file is opened for reading.
Signed-off-by: Paul Jackson <pj@sgi.com>
Signed-off-by: Simon Derr <simon.derr@bull.net>
Index: 2.6.9-rc1-mm4/kernel/cpuset.c
===================================================================
--- 2.6.9-rc1-mm4.orig/kernel/cpuset.c 2004-09-13 02:00:56.000000000 -0700
+++ 2.6.9-rc1-mm4/kernel/cpuset.c 2004-09-13 02:14:28.000000000 -0700
@@ -1017,7 +1017,7 @@ static int cpuset_add_file(struct dentry
* but we cannot guarantee that the information we produce is correct
* unless we produce it entirely atomically.
*
- * Upon first file read(), a struct ctr_struct is allocated, that
+ * Upon tasks file open(), a struct ctr_struct is allocated, that
* will have a pointer to an array (also allocated here). The struct
* ctr_struct * is stored in file->private_data. Its resources will
* be freed by release() when the file is closed. The array is used
@@ -1087,7 +1087,7 @@ static int pid_array_to_buf(char *buf, i
return cnt;
}
-static inline struct ctr_struct *cpuset_tasks_mkctr(struct file *file)
+static int cpuset_tasks_open(struct inode *unused, struct file *file)
{
struct cpuset *cs = __d_cs(file->f_dentry->d_parent);
struct ctr_struct *ctr;
@@ -1095,6 +1095,9 @@ static inline struct ctr_struct *cpuset_
int npids;
char c;
+ if (!(file->f_mode & FMODE_READ))
+ return 0;
+
ctr = kmalloc(sizeof(*ctr), GFP_KERNEL);
if (!ctr)
goto err0;
@@ -1122,14 +1125,14 @@ static inline struct ctr_struct *cpuset_
kfree(pidarray);
file->private_data = (void *)ctr;
- return ctr;
+ return 0;
err2:
kfree(pidarray);
err1:
kfree(ctr);
err0:
- return NULL;
+ return -ENOMEM;
}
static ssize_t cpuset_tasks_read(struct file *file, char __user *buf,
@@ -1137,13 +1140,6 @@ static ssize_t cpuset_tasks_read(struct
{
struct ctr_struct *ctr = (struct ctr_struct *)file->private_data;
- /* allocate buffer and fill it on first call to read() */
- if (!ctr) {
- ctr = cpuset_tasks_mkctr(file);
- if (!ctr)
- return -ENOMEM;
- }
-
if (*ppos + nbytes > ctr->bufsz)
nbytes = ctr->bufsz - *ppos;
if (copy_to_user(buf, ctr->buf + *ppos, nbytes))
@@ -1156,12 +1152,8 @@ static int cpuset_tasks_release(struct i
{
struct ctr_struct *ctr;
- /* we have nothing to do if no read-access is needed */
- if (!(file->f_mode & FMODE_READ))
- return 0;
-
- ctr = (struct ctr_struct *)file->private_data;
- if (ctr) {
+ if (file->f_mode & FMODE_READ) {
+ ctr = (struct ctr_struct *)file->private_data;
kfree(ctr->buf);
kfree(ctr);
}
@@ -1174,6 +1166,7 @@ static int cpuset_tasks_release(struct i
static struct cftype cft_tasks = {
.name = "tasks",
+ .open = cpuset_tasks_open,
.read = cpuset_tasks_read,
.release = cpuset_tasks_release,
.private = FILE_TASKLIST,
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.650.933.1373
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpusets: alternative fix for possible race in cpuset_tasks_read()
2004-09-13 9:25 ` Paul Jackson
@ 2004-09-13 9:39 ` Paul Jackson
0 siblings, 0 replies; 5+ messages in thread
From: Paul Jackson @ 2004-09-13 9:39 UTC (permalink / raw)
To: Paul Jackson; +Cc: derrs, simon.derr, akpm, linux-kernel
Aha - Andrew beat me to the mm5 draw.
I will send this patch again, shortly, against mm5, reversing the
affects of:
cpusets-fix-possible-race-in-cpuset_tasks_read.patch
and applying this alternative instead.
==> Ignore the patch I sent 15 minutes ago, with
Message-Id: <20040913022524.6d3b711e.pj@sgi.com>
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.650.933.1373
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-09-13 9:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-10 14:38 [PATCH] cpusets: fix possible race in cpuset_tasks_read() Simon Derr
2004-09-11 8:01 ` [PATCH] cpusets: alternative fix for " Paul Jackson
2004-09-13 8:24 ` Simon.Derr
2004-09-13 9:25 ` Paul Jackson
2004-09-13 9:39 ` Paul Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox