[Fix] Races in fs/super.c - please test

Alexander Viro (viro@math.psu.edu)
Fri, 23 Oct 1998 02:13:16 -0400 (EDT)


Folks, here's patch for some races in fs/super.c. vfsmnt_lookup(),
vfsmnt_remove(), vfsmnt_add() and get_filesystem_info() may interact ugly
and some of them _are_ callable at the same moment. I've added read/write
spinlock. I've also massaged vfsmntlist management - it simplified
add/remove.
I made get_fs_type() static - it is used _only_ in super.c (as in
"never mentioned outside of it").
I'm going to submit it to Linus if there will be no objections.

Patch (against 2.1.125, but should apply to pre-126 too) follows:

--- fs/super.c.orig Fri Oct 9 20:29:28 1998
+++ fs/super.c Thu Oct 22 14:53:45 1998
@@ -37,6 +37,7 @@
#include <asm/system.h>
#include <asm/uaccess.h>
#include <asm/bitops.h>
+#include <asm/spinlock.h>

#include <linux/nfs_fs.h>
#include <linux/nfs_fs_sb.h>
@@ -70,8 +71,9 @@

static struct file_system_type *file_systems = (struct file_system_type *) NULL;
struct vfsmount *vfsmntlist = (struct vfsmount *) NULL;
-static struct vfsmount *vfsmnttail = (struct vfsmount *) NULL,
- *mru_vfsmnt = (struct vfsmount *) NULL;
+static struct vfsmount *vfsmnttailp = &vfsmntlist;
+static struct vfsmount *mru_vfsmnt = (struct vfsmount *) NULL;
+static rwlock_t vfsmnt_lock = RW_LOCK_UNLOCKED;

/*
* This part handles the management of the list of mounted filesystems.
@@ -80,23 +82,22 @@
{
struct vfsmount *lptr;

- if (vfsmntlist == (struct vfsmount *)NULL)
- return ((struct vfsmount *)NULL);
+ if (!vfsmntlist)
+ return NULL;

- if (mru_vfsmnt != (struct vfsmount *)NULL &&
- mru_vfsmnt->mnt_dev == dev)
- return (mru_vfsmnt);
-
- for (lptr = vfsmntlist;
- lptr != (struct vfsmount *)NULL;
- lptr = lptr->mnt_next)
+ read_lock(vsfmnt_lock);
+
+ if (mru_vfsmnt && mru_vfsmnt->mnt_dev == dev)
+ lptr = mru_vfsmnt;
+ else for (lptr = vfsmntlist; lptr ; lptr = lptr->mnt_next)
if (lptr->mnt_dev == dev) {
mru_vfsmnt = lptr;
- return (lptr);
+ break;
}

- return ((struct vfsmount *)NULL);
- /* NOTREACHED */
+ read_unlock(vfsmnt_lock);
+
+ return lptr;
}

static struct vfsmount *add_vfsmnt(struct super_block *sb,
@@ -135,46 +136,39 @@
putname(tmp);
}

- if (vfsmntlist == (struct vfsmount *)NULL) {
- vfsmntlist = vfsmnttail = lptr;
- } else {
- vfsmnttail->mnt_next = lptr;
- vfsmnttail = lptr;
- }
+ write_lock(vfsmnt_lock);
+
+ *vfsmnttailp = lptr;
+ vfsmnttailp = &lptr->mnt_next;
+
+ write_unlock(vfsmnt_lock);
out:
return lptr;
}

static void remove_vfsmnt(kdev_t dev)
{
- struct vfsmount *lptr, *tofree;
+ struct vfsmount *p, **pp;

- if (vfsmntlist == (struct vfsmount *)NULL)
+ write_lock(vfsmnt_lock);
+
+ for(pp = &vfsmntlist; p=*pp && p->mnt_dev!=dev; pp = &p->mnt_next)
+ ;
+ if (!p) {
+ write_unlock(vfsmnt_lock);
return;
- lptr = vfsmntlist;
- if (lptr->mnt_dev == dev) {
- tofree = lptr;
- vfsmntlist = lptr->mnt_next;
- if (vfsmnttail->mnt_dev == dev)
- vfsmnttail = vfsmntlist;
- } else {
- while (lptr->mnt_next != (struct vfsmount *)NULL) {
- if (lptr->mnt_next->mnt_dev == dev)
- break;
- lptr = lptr->mnt_next;
- }
- tofree = lptr->mnt_next;
- if (tofree == (struct vfsmount *)NULL)
- return;
- lptr->mnt_next = lptr->mnt_next->mnt_next;
- if (vfsmnttail->mnt_dev == dev)
- vfsmnttail = lptr;
}
- if (tofree == mru_vfsmnt)
+ if (!(*pp = p->mnt_next)) {
+ vfsmnttailp = pp;
+ }
+ if (p == mru_vfsmnt)
mru_vfsmnt = NULL;
- kfree(tofree->mnt_devname);
- kfree(tofree->mnt_dirname);
- kfree_s(tofree, sizeof(struct vfsmount));
+
+ write_unlock(vfsmnt_lock);
+
+ kfree(p->mnt_devname);
+ kfree(p->mnt_dirname);
+ kfree_s(p, sizeof(struct vfsmount));
}

int register_filesystem(struct file_system_type * fs)
@@ -319,73 +313,63 @@

int get_filesystem_info( char *buf )
{
- struct vfsmount *tmp = vfsmntlist;
+ struct vfsmount *p;
struct proc_fs_info *fs_infop;
struct proc_nfs_info *nfs_infop;
struct nfs_server *nfss;
int len = 0;
+#define CAT(fmt,args...) len += sprintf(buf+len, fmt , ## args)
+
+ read_lock(vfsmnt_lock);
+
+ for(p = vfsmntlist; p && len < PAGE_SIZE - 160; p = p->mnt_next,
+ CAT(" 0 0\n") ) {
+ CAT("%s %s %s %s",
+ p->mnt_devname, p->mnt_dirname, p->mnt_sb->s_type->name,
+ p->mnt_flags & MS_RDONLY ? "ro" : "rw" );

- while ( tmp && len < PAGE_SIZE - 160)
- {
- len += sprintf( buf + len, "%s %s %s %s",
- tmp->mnt_devname, tmp->mnt_dirname, tmp->mnt_sb->s_type->name,
- tmp->mnt_flags & MS_RDONLY ? "ro" : "rw" );
for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
- if (tmp->mnt_flags & fs_infop->flag) {
- strcpy(buf + len, fs_infop->str);
- len += strlen(fs_infop->str);
- }
- }
- if (!strcmp("nfs", tmp->mnt_sb->s_type->name)) {
- nfss = &tmp->mnt_sb->u.nfs_sb.s_server;
- if (nfss->rsize != NFS_DEF_FILE_IO_BUFFER_SIZE) {
- len += sprintf(buf+len, ",rsize=%d",
- nfss->rsize);
- }
- if (nfss->wsize != NFS_DEF_FILE_IO_BUFFER_SIZE) {
- len += sprintf(buf+len, ",wsize=%d",
- nfss->wsize);
+ if (p->mnt_flags & fs_infop->flag) {
+ strcpy(buf + len, fs_infop->str);
+ len += strlen(fs_infop->str);
}
+ }
+
+ if (strcmp("nfs", p->mnt_sb->s_type->name) != 0)
+ continue;
+
+ nfss = &p->mnt_sb->u.nfs_sb.s_server;
+ if (nfss->rsize != NFS_DEF_FILE_IO_BUFFER_SIZE)
+ CAT(",rsize=%d", nfss->rsize);
+ if (nfss->wsize != NFS_DEF_FILE_IO_BUFFER_SIZE)
+ CAT(",wsize=%d", nfss->wsize);
#if 0
- if (nfss->timeo != 7*HZ/10) {
- len += sprintf(buf+len, ",timeo=%d",
- nfss->timeo*10/HZ);
- }
- if (nfss->retrans != 3) {
- len += sprintf(buf+len, ",retrans=%d",
- nfss->retrans);
- }
+ if (nfss->timeo != 7*HZ/10)
+ CAT(",timeo=%d", nfss->timeo*10/HZ);
+ if (nfss->retrans != 3)
+ CAT(",retrans=%d", nfss->retrans);
#endif
- if (nfss->acregmin != 3*HZ) {
- len += sprintf(buf+len, ",acregmin=%d",
- nfss->acregmin/HZ);
- }
- if (nfss->acregmax != 60*HZ) {
- len += sprintf(buf+len, ",acregmax=%d",
- nfss->acregmax/HZ);
- }
- if (nfss->acdirmin != 30*HZ) {
- len += sprintf(buf+len, ",acdirmin=%d",
- nfss->acdirmin/HZ);
+ if (nfss->acregmin != 3*HZ)
+ CAT(",acregmin=%d", nfss->acregmin/HZ);
+ if (nfss->acregmax != 60*HZ)
+ CAT(",acregmax=%d", nfss->acregmax/HZ);
+ if (nfss->acdirmin != 30*HZ)
+ CAT(",acdirmin=%d", nfss->acdirmin/HZ);
+ if (nfss->acdirmax != 60*HZ)
+ CAT(",acdirmax=%d", nfss->acdirmax/HZ);
+ for (nfs_infop = nfs_info; nfs_infop->flag; nfs_infop++) {
+ if (nfss->flags & nfs_infop->flag) {
+ strcpy(buf + len, nfs_infop->str);
+ len += strlen(nfs_infop->str);
}
- if (nfss->acdirmax != 60*HZ) {
- len += sprintf(buf+len, ",acdirmax=%d",
- nfss->acdirmax/HZ);
- }
- for (nfs_infop = nfs_info; nfs_infop->flag; nfs_infop++) {
- if (nfss->flags & nfs_infop->flag) {
- strcpy(buf + len, nfs_infop->str);
- len += strlen(nfs_infop->str);
- }
- }
- len += sprintf(buf+len, ",addr=%s",
- nfss->hostname);
}
- len += sprintf( buf + len, " 0 0\n" );
- tmp = tmp->mnt_next;
+ CAT(",addr=%s", nfss->hostname);
}

+ read_unlock(vfsmnt_lock);
+
return len;
+#undef CAT
}

int get_filesystem_list(char * buf)
@@ -403,7 +387,7 @@
return len;
}

-struct file_system_type *get_fs_type(const char *name)
+static struct file_system_type *get_fs_type(const char *name)
{
struct file_system_type * fs = file_systems;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/