Re: [PATCH] 2.4.13pre3aa1: expand_fdset() may use invalid pointer

Maneesh Soni
2001-10-18T06:36:23Z

On Wed, Oct 17, 2001 at 08:42:04PM +0200, Andrea Arcangeli wrote:
> On Wed, Oct 17, 2001 at 11:32:45AM -0700, Chip Salzenberg wrote:
> > In 2.4.13pre3aa1, expand_fdset() in fs/file.c has a couple of
> > execution paths that call kfree() on a pointer that hasn't yet been
> > initialized. A minimal patch is attached.
>
> Good spotting! Thanks for the fix!! applied.
>
> CC'ed Maneesh since he's maintaining the rcu_fdset patch AFIK.
>
> > --
> > Chip Salzenberg - a.k.a. - <chip@pobox.com>
> > "We have no fuel on board, plus or minus 8 kilograms." -- NEAR tech
>
> >
> > Index: linux/fs/file.c
> > --- linux/fs/file.c.old Tue Oct 16 23:28:16 2001
> > +++ linux/fs/file.c Wed Oct 17 00:29:43 2001
> > @@ -203,5 +203,5 @@
> > fd_set *new_openset = 0, *new_execset = 0;
> > int error, nfds = 0;
> > - struct rcu_fd_set *arg;
> > + struct rcu_fd_set *arg = NULL;
> >
> > error = -EMFILE;

I also removed some un-necessary diffs from the patch. The updated patch is as
below.

Thanks,
Maneesh

--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5262355 Extn. 3999 email: maneesh@in.ibm.com
http://lse.sourceforge.net/locking/rcupdate.html

diff -urN linux-2.4.12/drivers/char/tty_io.c linux-2.4.12-fs-05/drivers/char/tty_io.c
--- linux-2.4.12/drivers/char/tty_io.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/drivers/char/tty_io.c Thu Oct 18 10:33:41 2001
@@ -1847,7 +1847,6 @@
}
task_lock(p);
if (p->files) {
- read_lock(&p->files->file_lock);
for (i=0; i < p->files->max_fds; i++) {
filp = fcheck_files(p->files, i);
if (filp && (filp->f_op == &tty_fops) &&
@@ -1856,7 +1855,6 @@
break;
}
}
- read_unlock(&p->files->file_lock);
}
task_unlock(p);
}
diff -urN linux-2.4.12/fs/exec.c linux-2.4.12-fs-05/fs/exec.c
--- linux-2.4.12/fs/exec.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/exec.c Thu Oct 18 10:33:41 2001
@@ -482,7 +482,7 @@
{
long j = -1;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
for (;;) {
unsigned long set, i;

@@ -494,16 +494,16 @@
if (!set)
continue;
files->close_on_exec->fds_bits[j] = 0;
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
for ( ; set ; i++,set >>= 1) {
if (set & 1) {
sys_close(i);
}
}
- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);

}
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
}

/*
diff -urN linux-2.4.12/fs/fcntl.c linux-2.4.12-fs-05/fs/fcntl.c
--- linux-2.4.12/fs/fcntl.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/fcntl.c Thu Oct 18 10:33:41 2001
@@ -64,7 +64,7 @@
int error;
int start;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);

repeat:
/*
@@ -110,7 +110,7 @@
{
FD_SET(fd, files->open_fds);
FD_CLR(fd, files->close_on_exec);
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
fd_install(fd, file);
}

@@ -126,7 +126,7 @@
return ret;

out_putf:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
fput(file);
return ret;
}
@@ -137,7 +137,7 @@
struct file * file, *tofree;
struct files_struct * files = current->files;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
if (!(file = fcheck(oldfd)))
goto out_unlock;
err = newfd;
@@ -168,7 +168,7 @@
files->fd[newfd] = file;
FD_SET(newfd, files->open_fds);
FD_CLR(newfd, files->close_on_exec);
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);

if (tofree)
filp_close(tofree, files);
@@ -176,11 +176,11 @@
out:
return err;
out_unlock:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
goto out;

out_fput:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
fput(file);
goto out;
}
diff -urN linux-2.4.12/fs/file.c linux-2.4.12-fs-05/fs/file.c
--- linux-2.4.12/fs/file.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/file.c Thu Oct 18 11:24:03 2001
@@ -13,7 +13,20 @@
#include <linux/vmalloc.h>

#include <asm/bitops.h>
+#include <linux/rcupdate.h>

+struct rcu_fd_array {
+ struct rcu_head rh;
+ struct file **array;
+ int nfds;
+};
+
+struct rcu_fd_set {
+ struct rcu_head rh;
+ fd_set *openset;
+ fd_set *execset;
+ int nfds;
+};

/*
* Allocate an fd array, using kmalloc or vmalloc.
@@ -48,6 +61,13 @@
vfree(array);
}

+static void fd_array_callback(void *arg)
+{
+ struct rcu_fd_array *a = (struct rcu_fd_array *) arg;
+ free_fd_array(a->array, a->nfds);
+ kfree(arg);
+}
+
/*
* Expand the fd array in the files_struct. Called with the files
* spinlock held for write.
@@ -57,6 +77,7 @@
{
struct file **new_fds;
int error, nfds;
+ struct rcu_fd_array *arg;

error = -EMFILE;
@@ -64,7 +85,7 @@
goto out;

nfds = files->max_fds;
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);

/*
* Expand to the max in easy steps, and keep expanding it until
@@ -88,18 +109,17 @@

error = -ENOMEM;
new_fds = alloc_fd_array(nfds);
- write_lock(&files->file_lock);
- if (!new_fds)
+ arg = (struct rcu_fd_array *) kmalloc(sizeof(arg), GFP_ATOMIC);
+
+ spin_lock(&files->file_lock);
+ if (!new_fds || !arg)
goto out;

/* Copy the existing array and install the new pointer */

if (nfds > files->max_fds) {
- struct file **old_fds;
- int i;
-
- old_fds = xchg(&files->fd, new_fds);
- i = xchg(&files->max_fds, nfds);
+ struct file **old_fds = files->fd;
+ int i = files->max_fds;

/* Don't copy/clear the array if we are creating a new
fd array for fork() */
@@ -108,16 +128,27 @@
/* clear the remainder of the array */
memset(&new_fds[i], 0,
(nfds-i) * sizeof(struct file *));
+ }

- write_unlock(&files->file_lock);
- free_fd_array(old_fds, i);
- write_lock(&files->file_lock);
+ /* mem barrier needed for Alpha*/
+ files->fd = new_fds;
+ /* mem barrier needed for Alpha*/
+ files->max_fds = nfds;
+
+ if (i) {
+ arg->array = old_fds;
+ arg->nfds = i;
+ call_rcu(&arg->rh, fd_array_callback, arg);
+ }
+ else {
+ kfree(arg);
}
} else {
/* Somebody expanded the array while we slept ... */
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
free_fd_array(new_fds, nfds);
- write_lock(&files->file_lock);
+ kfree(arg);
+ spin_lock(&files->file_lock);
}
error = 0;
out:
@@ -157,6 +188,14 @@
vfree(array);
}

+static void fd_set_callback (void *arg)
+{
+ struct rcu_fd_set *a = (struct rcu_fd_set *) arg;
+ free_fdset(a->openset, a->nfds);
+ free_fdset(a->execset, a->nfds);
+ kfree(arg);
+}
+
/*
* Expand the fdset in the files_struct. Called with the files spinlock
* held for write.
@@ -165,13 +204,14 @@
{
fd_set *new_openset = 0, *new_execset = 0;
int error, nfds = 0;
+ struct rcu_fd_set *arg = NULL;

error = -EMFILE;
if (files->max_fdset >= NR_OPEN || nr >= NR_OPEN)
goto out;

nfds = files->max_fdset;
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);

/* Expand to the max in easy steps */
do {
@@ -187,46 +227,56 @@
error = -ENOMEM;
new_openset = alloc_fdset(nfds);
new_execset = alloc_fdset(nfds);
- write_lock(&files->file_lock);
- if (!new_openset || !new_execset)
+ arg = (struct rcu_fd_set *) kmalloc(sizeof(arg), GFP_ATOMIC);
+ spin_lock(&files->file_lock);
+ if (!new_openset || !new_execset || !arg)
goto out;

error = 0;

/* Copy the existing tables and install the new pointers */
if (nfds > files->max_fdset) {
- int i = files->max_fdset / (sizeof(unsigned long) * 8);
- int count = (nfds - files->max_fdset) / 8;
+ fd_set * old_openset = files->open_fds;
+ fd_set * old_execset = files->close_on_exec;
+ int old_nfds = files->max_fdset;
+ int i = old_nfds / (sizeof(unsigned long) * 8);
+ int count = (nfds - old_nfds) / 8;

/*
* Don't copy the entire array if the current fdset is
* not yet initialised.
*/
if (i) {
- memcpy (new_openset, files->open_fds, files->max_fdset/8);
- memcpy (new_execset, files->close_on_exec, files->max_fdset/8);
+ memcpy (new_openset, old_openset, old_nfds/8);
+ memcpy (new_execset, old_execset, old_nfds/8);
memset (&new_openset->fds_bits[i], 0, count);
memset (&new_execset->fds_bits[i], 0, count);
}

- nfds = xchg(&files->max_fdset, nfds);
- new_openset = xchg(&files->open_fds, new_openset);
- new_execset = xchg(&files->close_on_exec, new_execset);
- write_unlock(&files->file_lock);
- free_fdset (new_openset, nfds);
- free_fdset (new_execset, nfds);
- write_lock(&files->file_lock);
+ /* mem barrier needed for Alpha*/
+ files->open_fds = new_openset;
+ files->close_on_exec = new_execset;
+ /* mem barrier needed for Alpha*/
+ files->max_fdset = nfds;
+
+ arg->openset = old_openset;
+ arg->execset = old_execset;
+ arg->nfds = nfds;
+ call_rcu(&arg->rh, fd_set_callback, arg);
+
return 0;
}
/* Somebody expanded the array while we slept ... */

out:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
if (new_openset)
free_fdset(new_openset, nfds);
if (new_execset)
free_fdset(new_execset, nfds);
- write_lock(&files->file_lock);
+ if (arg)
+ kfree(arg);
+ spin_lock(&files->file_lock);
return error;
}

diff -urN linux-2.4.12/fs/file_table.c linux-2.4.12-fs-05/fs/file_table.c
--- linux-2.4.12/fs/file_table.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/file_table.c Thu Oct 18 10:33:41 2001
@@ -129,13 +129,22 @@
struct file * fget(unsigned int fd)
{
struct file * file;
- struct files_struct *files = current->files;

- read_lock(&files->file_lock);
file = fcheck(fd);
- if (file)
+ if (file) {
get_file(file);
- read_unlock(&files->file_lock);
+
+ /* before returning check again if someone (as of now sys_close)
+ * has nullified the fd_array entry, if yes then we might have
+ * failed fput call for him by doing get_file() so do the
+ * favour of doing fput for him.
+ */
+
+ if (!(fcheck(fd))) {
+ fput(file);
+ return NULL;
+ }
+ }
return file;
}

diff -urN linux-2.4.12/fs/open.c linux-2.4.12-fs-05/fs/open.c
--- linux-2.4.12/fs/open.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/open.c Thu Oct 18 10:33:41 2001
@@ -702,7 +702,7 @@
int fd, error;

error = -EMFILE;
- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);

repeat:
fd = find_next_zero_bit(files->open_fds,
@@ -751,7 +751,7 @@
error = fd;

out:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
return error;
}

@@ -832,7 +832,7 @@
struct file * filp;
struct files_struct *files = current->files;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
if (fd >= files->max_fds)
goto out_unlock;
filp = files->fd[fd];
@@ -841,11 +841,11 @@
files->fd[fd] = NULL;
FD_CLR(fd, files->close_on_exec);
__put_unused_fd(files, fd);
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
return filp_close(filp, files);

out_unlock:
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
return -EBADF;
}

diff -urN linux-2.4.12/fs/proc/base.c linux-2.4.12-fs-05/fs/proc/base.c
--- linux-2.4.12/fs/proc/base.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/proc/base.c Thu Oct 18 10:33:41 2001
@@ -754,12 +754,10 @@
task_unlock(task);
if (!files)
goto out_unlock;
- read_lock(&files->file_lock);
file = inode->u.proc_i.file = fcheck_files(files, fd);
if (!file)
goto out_unlock2;
get_file(file);
- read_unlock(&files->file_lock);
put_files_struct(files);
inode->i_op = &proc_pid_link_inode_operations;
inode->i_size = 64;
@@ -775,7 +773,6 @@

out_unlock2:
put_files_struct(files);
- read_unlock(&files->file_lock);
out_unlock:
iput(inode);
out:
diff -urN linux-2.4.12/fs/select.c linux-2.4.12-fs-05/fs/select.c
--- linux-2.4.12/fs/select.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/fs/select.c Thu Oct 18 10:33:41 2001
@@ -167,9 +167,7 @@
int retval, i, off;
long __timeout = *timeout;

- read_lock(&current->files->file_lock);
retval = max_select_fd(n, fds);
- read_unlock(&current->files->file_lock);

if (retval < 0)
return retval;
diff -urN linux-2.4.12/include/linux/file.h linux-2.4.12-fs-05/include/linux/file.h
--- linux-2.4.12/include/linux/file.h Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/include/linux/file.h Thu Oct 18 10:33:41 2001
@@ -12,21 +12,19 @@
{
struct files_struct *files = current->files;
int res;
- read_lock(&files->file_lock);
res = FD_ISSET(fd, files->close_on_exec);
- read_unlock(&files->file_lock);
return res;
}

static inline void set_close_on_exec(unsigned int fd, int flag)
{
struct files_struct *files = current->files;
- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
if (flag)
FD_SET(fd, files->close_on_exec);
else
FD_CLR(fd, files->close_on_exec);
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
}

static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
@@ -66,9 +64,9 @@
{
struct files_struct *files = current->files;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
__put_unused_fd(files, fd);
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
}

/*
@@ -88,11 +86,11 @@
{
struct files_struct *files = current->files;

- write_lock(&files->file_lock);
+ spin_lock(&files->file_lock);
if (files->fd[fd])
BUG();
files->fd[fd] = file;
- write_unlock(&files->file_lock);
+ spin_unlock(&files->file_lock);
}

void put_files_struct(struct files_struct *fs);
diff -urN linux-2.4.12/include/linux/sched.h linux-2.4.12-fs-05/include/linux/sched.h
--- linux-2.4.12/include/linux/sched.h Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/include/linux/sched.h Thu Oct 18 11:13:12 2001
@@ -171,7 +171,7 @@
*/
struct files_struct {
atomic_t count;
- rwlock_t file_lock; /* Protects all the below members. Nests inside tsk->alloc_lock */
+ spinlock_t file_lock; /* Protects all the below members. Nests inside tsk->alloc_lock */
int max_fds;
int max_fdset;
int next_fd;
@@ -186,7 +186,7 @@
#define INIT_FILES \
{ \
count: ATOMIC_INIT(1), \
- file_lock: RW_LOCK_UNLOCKED, \
+ file_lock: SPIN_LOCK_UNLOCKED, \
max_fds: NR_OPEN_DEFAULT, \
max_fdset: __FD_SETSIZE, \
next_fd: 0, \
diff -urN linux-2.4.12/kernel/fork.c linux-2.4.12-fs-05/kernel/fork.c
--- linux-2.4.12/kernel/fork.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/kernel/fork.c Thu Oct 18 11:44:45 2001
@@ -440,7 +440,7 @@

atomic_set(&newf->count, 1);

- newf->file_lock = RW_LOCK_UNLOCKED;
+ newf->file_lock = SPIN_LOCK_UNLOCKED;
newf->next_fd = 0;
newf->max_fds = NR_OPEN_DEFAULT;
newf->max_fdset = __FD_SETSIZE;
@@ -453,13 +453,12 @@
size = oldf->max_fdset;
if (size > __FD_SETSIZE) {
newf->max_fdset = 0;
- write_lock(&newf->file_lock);
+ spin_lock(&newf->file_lock);
error = expand_fdset(newf, size-1);
- write_unlock(&newf->file_lock);
+ spin_unlock(&newf->file_lock);
if (error)
goto out_release;
}
- read_lock(&oldf->file_lock);

open_files = count_open_files(oldf, size);

@@ -470,15 +469,13 @@
*/
nfds = NR_OPEN_DEFAULT;
if (open_files > nfds) {
- read_unlock(&oldf->file_lock);
newf->max_fds = 0;
- write_lock(&newf->file_lock);
+ spin_lock(&newf->file_lock);
error = expand_fd_array(newf, open_files-1);
- write_unlock(&newf->file_lock);
+ spin_unlock(&newf->file_lock);
if (error)
goto out_release;
nfds = newf->max_fds;
- read_lock(&oldf->file_lock);
}

old_fds = oldf->fd;
@@ -493,7 +490,6 @@
get_file(f);
*new_fds++ = f;
}
- read_unlock(&oldf->file_lock);

/* compute the remainder to be cleared */
size = (newf->max_fds - open_files) * sizeof(struct file *);
diff -urN linux-2.4.12/net/ipv4/netfilter/ipt_owner.c linux-2.4.12-fs-05/net/ipv4/netfilter/ipt_owner.c
--- linux-2.4.12/net/ipv4/netfilter/ipt_owner.c Thu Oct 18 11:44:21 2001
+++ linux-2.4.12-fs-05/net/ipv4/netfilter/ipt_owner.c Thu Oct 18 10:33:41 2001
@@ -25,16 +25,13 @@
task_lock(p);
files = p->files;
if(files) {
- read_lock(&files->file_lock);
for (i=0; i < files->max_fds; i++) {
if (fcheck_files(files, i) == skb->sk->socket->file) {
- read_unlock(&files->file_lock);
task_unlock(p);
read_unlock(&tasklist_lock);
return 1;
}
}
- read_unlock(&files->file_lock);
}
task_unlock(p);
out:
@@ -58,14 +55,12 @@
task_lock(p);
files = p->files;
if (files) {
- read_lock(&files->file_lock);
for (i=0; i < files->max_fds; i++) {
if (fcheck_files(files, i) == file) {
found = 1;
break;
}
}
- read_unlock(&files->file_lock);
}
task_unlock(p);
if(found)