Hello,
I think there is a possibility for two threads from a single process to
race in sysfs_read_file() if they call read() on the same file at the same
time.
Supposing no read() has been done on this file previously, the two threads
could end allocating each a new page in fill_read_buffer() for
buffer->page, and one of these pages would be lost forever.
The same applies to write().
This patch adds a semaphore in struct sysfs_buffer to fix this issue.
Now it should be sufficient, to fix this memory leak, to protect only the
call to get_zeroed_page() in fill_read_buffer() with the semaphore.
Instead, this patch holds the semaphore during the whole
sysfs_read_file().
The reason is that there is also a risk, with the current code, of having
one thread in flush_read_buffer() while the another is in
fill_read_buffer()/ops->show(), and then maybe read() returning garbage.
This is more likely to happen actually if two threads call write() at the
same time with different data, with one thread using the data of the
buffer->page in ops->store(), and another thread overwriting that data in
fill_write_buffer(), thus maybe having ops->store() using corrupted data.
Of course in this case, the multithreaded application is broken and should
not expect a correct kernel behaviour... so maybe only protection around
get_zeroed_page() is needed.
Simon.
This patch is against 2.6.9-rc1-mm2.
Signed-off-by: Simon Derr <[email protected]>
Index: 269mm2kdb/fs/sysfs/file.c
===================================================================
--- 269mm2kdb.orig/fs/sysfs/file.c 2004-08-31 11:29:16.000000000 +0200
+++ 269mm2kdb/fs/sysfs/file.c 2004-09-01 10:00:57.582251616 +0200
@@ -6,6 +6,7 @@
#include <linux/dnotify.h>
#include <linux/kobject.h>
#include <asm/uaccess.h>
+#include <asm/semaphore.h>
#include "sysfs.h"
@@ -53,6 +54,7 @@
loff_t pos;
char * page;
struct sysfs_ops * ops;
+ struct semaphore sem;
};
@@ -140,13 +142,17 @@
struct sysfs_buffer * buffer = file->private_data;
ssize_t retval = 0;
+ down(&buffer->sem);
if (!*ppos) {
if ((retval = fill_read_buffer(file->f_dentry,buffer)))
- return retval;
+ goto out;
}
pr_debug("%s: count = %d, ppos = %lld, buf = %s\n",
__FUNCTION__,count,*ppos,buffer->page);
- return flush_read_buffer(buffer,buf,count,ppos);
+ retval = flush_read_buffer(buffer,buf,count,ppos);
+out:
+ up(&buffer->sem);
+ return retval;
}
@@ -220,11 +226,13 @@
{
struct sysfs_buffer * buffer = file->private_data;
+ down(&buffer->sem);
count = fill_write_buffer(buffer,buf,count);
if (count > 0)
count = flush_write_buffer(file->f_dentry,buffer,count);
if (count > 0)
*ppos += count;
+ up(&buffer->sem);
return count;
}
@@ -287,6 +295,7 @@
buffer = kmalloc(sizeof(struct sysfs_buffer),GFP_KERNEL);
if (buffer) {
memset(buffer,0,sizeof(struct sysfs_buffer));
+ init_MUTEX(&buffer->sem);
buffer->ops = ops;
file->private_data = buffer;
} else
Simon Derr <[email protected]> wrote:
>
> I think there is a possibility for two threads from a single process to
> race in sysfs_read_file() if they call read() on the same file at the same
> time.
I think there is, too.
I also wonder what happens if the first read of a sysfs file is not at
offset zero (eg: pread()):
static ssize_t
sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
struct sysfs_buffer * buffer = file->private_data;
ssize_t retval = 0;
if (!*ppos) {
if ((retval = fill_read_buffer(file->f_dentry,buffer)))
we seem to not allocate the buffer at all?
On Wed, 1 Sep 2004, Andrew Morton wrote:
> Simon Derr <[email protected]> wrote:
> >
> > I think there is a possibility for two threads from a single process to
> > race in sysfs_read_file() if they call read() on the same file at the same
> > time.
>
> I think there is, too.
>
> I also wonder what happens if the first read of a sysfs file is not at
> offset zero (eg: pread()):
>
> static ssize_t
> sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> struct sysfs_buffer * buffer = file->private_data;
> ssize_t retval = 0;
>
> if (!*ppos) {
> if ((retval = fill_read_buffer(file->f_dentry,buffer)))
>
>
> we seem to not allocate the buffer at all?
Indeed.
And one would expect flush_read_buffer() to handle this by returning zero
since the buffer has not been allocated, and its length is zero. But it
seems there is also something wrong in flush_read_buffer().
fs/sysfs/file.c, flush_read_buffer():
if (count > (buffer->count - *ppos))
count = buffer->count - *ppos;
error = copy_to_user(buf,buffer->page + *ppos,count);
Several issues:
* case 1: the buffer has been previously allocated by a call to read(),
but now we call pread() (or have called lseek()) with a "large" offset:
Let's say buffer->count is 20 and *ppos is 1000:
buffer->count - *ppos is negative, but buffer->count is unsigned, so there
is an overflow and the comparison (count > (buffer->count - *ppos)) is
false, even though `count' is bigger than `buffer->count'
The user here can call copy_to_user() with about any length he wants.
* case 2: the buffer has not yet been allocated, and we call pread() with
a non-null offset. Due to the overflow, we can again call copy_to_user
with about any length we want (`count'), from about any location we want
(`*ppos')
Here's an updated patch:
1/fixes the race between threads by adding a semaphore in sysfs_buffer
2/allocates the buffer upon call to pread(). We still call again
fill_read_buffer() if the file is "rewinded" to offset zero.
3/fixes the comparison in flush_read_buffer().
Still against 2.6.9-rc1-mm2.
Signed-off-by: Simon Derr <[email protected]>
Index: 269mm2kdb/fs/sysfs/file.c
===================================================================
--- 269mm2kdb.orig/fs/sysfs/file.c 2004-08-31 11:29:16.000000000 +0200
+++ 269mm2kdb/fs/sysfs/file.c 2004-09-02 10:58:52.344480007 +0200
@@ -6,6 +6,7 @@
#include <linux/dnotify.h>
#include <linux/kobject.h>
#include <asm/uaccess.h>
+#include <asm/semaphore.h>
#include "sysfs.h"
@@ -53,6 +54,7 @@
loff_t pos;
char * page;
struct sysfs_ops * ops;
+ struct semaphore sem;
};
@@ -106,6 +108,9 @@
{
int error;
+ if (*ppos > buffer->count)
+ return 0;
+
if (count > (buffer->count - *ppos))
count = buffer->count - *ppos;
@@ -140,13 +145,17 @@
struct sysfs_buffer * buffer = file->private_data;
ssize_t retval = 0;
- if (!*ppos) {
+ down(&buffer->sem);
+ if ((!*ppos) || (!buffer->page)) {
if ((retval = fill_read_buffer(file->f_dentry,buffer)))
- return retval;
+ goto out;
}
pr_debug("%s: count = %d, ppos = %lld, buf = %s\n",
__FUNCTION__,count,*ppos,buffer->page);
- return flush_read_buffer(buffer,buf,count,ppos);
+ retval = flush_read_buffer(buffer,buf,count,ppos);
+out:
+ up(&buffer->sem);
+ return retval;
}
@@ -220,11 +229,13 @@
{
struct sysfs_buffer * buffer = file->private_data;
+ down(&buffer->sem);
count = fill_write_buffer(buffer,buf,count);
if (count > 0)
count = flush_write_buffer(file->f_dentry,buffer,count);
if (count > 0)
*ppos += count;
+ up(&buffer->sem);
return count;
}
@@ -287,6 +298,7 @@
buffer = kmalloc(sizeof(struct sysfs_buffer),GFP_KERNEL);
if (buffer) {
memset(buffer,0,sizeof(struct sysfs_buffer));
+ init_MUTEX(&buffer->sem);
buffer->ops = ops;
file->private_data = buffer;
} else
Simon Derr <[email protected]> wrote:
>
> @@ -140,13 +145,17 @@
> struct sysfs_buffer * buffer = file->private_data;
> ssize_t retval = 0;
>
> - if (!*ppos) {
> + down(&buffer->sem);
> + if ((!*ppos) || (!buffer->page)) {
> if ((retval = fill_read_buffer(file->f_dentry,buffer)))
> - return retval;
> + goto out;
Why are we testing *ppos at all in here?
On Thu, 2 Sep 2004, Andrew Morton wrote:
> Simon Derr <[email protected]> wrote:
> >
> > @@ -140,13 +145,17 @@
> > struct sysfs_buffer * buffer = file->private_data;
> > ssize_t retval = 0;
> >
> > - if (!*ppos) {
> > + down(&buffer->sem);
> > + if ((!*ppos) || (!buffer->page)) {
> > if ((retval = fill_read_buffer(file->f_dentry,buffer)))
> > - return retval;
> > + goto out;
>
> Why are we testing *ppos at all in here?
To keep the original behaviour, that is, after lseek() to offset zero,
ops->show() is called again and the contents of the file are refreshed.
Some applications maybe rely on it, or it may be completely useless, I
don't known.
And if we open a sysfs file with O_RDWR, and write() into it, then this is
needed if we want to read(), because else the buffer will have been
allocated, but ops->show() not called. I'm not too sure about this being
useful either.
PS: just in case my previous patch is of any interest, note that I made an
interesting typo in the 'Signed-off-by' email field.
On Fri, 3 Sep 2004, Simon Derr wrote:
> On Thu, 2 Sep 2004, Andrew Morton wrote:
>
> > Simon Derr <[email protected]> wrote:
> > >
> > > @@ -140,13 +145,17 @@
> > > struct sysfs_buffer * buffer = file->private_data;
> > > ssize_t retval = 0;
> > >
> > > - if (!*ppos) {
> > > + down(&buffer->sem);
> > > + if ((!*ppos) || (!buffer->page)) {
> > > if ((retval = fill_read_buffer(file->f_dentry,buffer)))
> > > - return retval;
> > > + goto out;
> >
> > Why are we testing *ppos at all in here?
>
> And if we open a sysfs file with O_RDWR, and write() into it, then this is
> needed if we want to read(), because else the buffer will have been
> allocated, but ops->show() not called. I'm not too sure about this being
> useful either.
Hmmm...
We are still screwed if we read at offset >0 after a write.
Possible fix would be to add a 'dirty' flag to the sysfs_buffer when
write() is called, so we force a call to fill_read_buffer() on the next
read().
Or maybe simply forbid O_RDWR open() ? Are they of any use ?
On Fri, 3 Sep 2004, Simon Derr wrote:
> Possible fix would be to add a 'dirty' flag to the sysfs_buffer when
> write() is called, so we force a call to fill_read_buffer() on the next
> read().
This additional patch does just that.
I'm just afraid the flag has not a very good name.
Signed-off-by: Simon Derr <[email protected]>
Index: 269mm2kdb/fs/sysfs/file.c
===================================================================
--- 269mm2kdb.orig/fs/sysfs/file.c 2004-09-03 11:43:01.744675609 +0200
+++ 269mm2kdb/fs/sysfs/file.c 2004-09-03 11:56:06.993689427 +0200
@@ -55,6 +55,7 @@
char * page;
struct sysfs_ops * ops;
struct semaphore sem;
+ int needs_read_fill;
};
@@ -82,6 +83,7 @@
return -ENOMEM;
count = ops->show(kobj,attr,buffer->page);
+ buffer->needs_read_fill = 0;
BUG_ON(count > (ssize_t)PAGE_SIZE);
if (count >= 0)
buffer->count = count;
@@ -146,7 +148,7 @@
ssize_t retval = 0;
down(&buffer->sem);
- if ((!*ppos) || (!buffer->page)) {
+ if (buffer->needs_read_fill) {
if ((retval = fill_read_buffer(file->f_dentry,buffer)))
goto out;
}
@@ -182,6 +184,7 @@
if (count >= PAGE_SIZE)
count = PAGE_SIZE - 1;
error = copy_from_user(buffer->page,buf,count);
+ buffer->needs_read_fill = 1;
return error ? -EFAULT : count;
}
@@ -299,6 +302,7 @@
if (buffer) {
memset(buffer,0,sizeof(struct sysfs_buffer));
init_MUTEX(&buffer->sem);
+ buffer->needs_read_fill = 1;
buffer->ops = ops;
file->private_data = buffer;
} else