patch for fs/dcache race

Bill Hawes (whawes@star.net)
Sat, 09 Aug 1997 18:00:21 -0400


This is a multi-part message in MIME format.
--------------9BBC83636660EF090DE14540
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

It seems that the kswapd oops bug is still extant, so I checked some
more and found a race in d_move that could account for the problem. The
call to d_remove_from_parent() dputs the parent dentry, and a call to
alloc_new_name is made before the new parent pointer is installed. If
alloc_new_name has to get more memory, it could start the kswapd
process, and a shrink_dcache could then catch the dentry with a stale
parent pointer, leading to the oops.

The attached patch fixes this by replacing d_remove_from_parent with
d_drop, followed by saving the old parent and installing the new. The
(possibly blocking) dput is deferred until the dentry is hashed into the
new parent.

I've also incorporated the changes to fs/dcache.c and fs/namei.c
following our discussion yesterday. Dentries are now delivered from
d_alloc with d_count = 1 and a valid parent pointer -- either self or a
dget(parent). This keeps the dentry safe for the duration of the fs
lookup, and real_lookup now delivers the dentry with the use count
already incremented.

One further improvement I'd like to see would be to pass a reference to
d_move of a dentry with the new name. It could then just swap names in
the dentries instead of having to allocate. Does this seem reasonable?

Regards,
Bill
--------------9BBC83636660EF090DE14540
Content-Type: text/plain; charset=us-ascii; name="dcache_48-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="dcache_48-patch"

--- linux-2.1.48/fs/dcache.c.old Tue Aug 5 10:46:00 1997
+++ linux-2.1.48/fs/dcache.c Sat Aug 9 17:16:07 1997
@@ -121,7 +121,10 @@
}
parent = dentry->d_parent;
d_free(dentry);
- dput(parent);
+ if (dentry != parent)
+ dput(parent);
+ else
+ printk("shrink_dcache: no parent??\n");
}
}
}
@@ -146,10 +149,10 @@
memcpy(str, name->name, name->len);
str[name->len] = 0;

- dentry->d_count = 0;
+ dentry->d_count = 1;
dentry->d_flags = 0;
dentry->d_inode = NULL;
- dentry->d_parent = parent;
+ dentry->d_parent = parent ? dget(parent) : dentry;
dentry->d_mounts = dentry;
dentry->d_covers = dentry;
INIT_LIST_HEAD(&dentry->d_hash);
@@ -187,9 +190,8 @@

if (root_inode) {
res = d_alloc(NULL, &(const struct qstr) { "/", 1, 0 });
- res->d_parent = res;
- res->d_count = 1;
- d_instantiate(res, root_inode);
+ if (res)
+ d_instantiate(res, root_inode);
}
return res;
}
@@ -272,16 +274,9 @@

static inline void d_insert_to_parent(struct dentry * entry, struct dentry * parent)
{
- list_add(&entry->d_hash, d_hash(dget(parent), entry->d_name.hash));
+ list_add(&entry->d_hash, d_hash(parent, entry->d_name.hash));
}

-static inline void d_remove_from_parent(struct dentry * dentry, struct dentry * parent)
-{
- list_del(&dentry->d_hash);
- dput(parent);
-}
-
-
/*
* When a file is deleted, we have two options:
* - turn this dentry into a negative dentry
@@ -340,19 +337,34 @@
entry->d_name.hash = hash;
}

+/*
+ * N.B. alloc_new_name could fail, so this shouldn't return void
+ * Even better, pass a reference to a dentry with the new name,
+ * and swap names instead of allocating a new one.
+ */
void d_move(struct dentry * dentry, struct dentry * newdir, struct qstr * newname)
{
+ struct dentry * old_parent;
if (!dentry)
return;

if (!dentry->d_inode)
printk("VFS: moving negative dcache entry\n");

- d_remove_from_parent(dentry, dentry->d_parent);
+ if (dentry == newdir)
+ printk("VFS: self-parented dentry %s\n", dentry->d_name.name);
+
+ d_drop(dentry);
+ /*
+ * To avoid races, set up the new parent before dputting the old.
+ */
+ old_parent = dentry->d_parent;
+ dentry->d_parent = dget(newdir);
alloc_new_name(dentry, newname);
- dentry->d_parent = newdir;
d_insert_to_parent(dentry, newdir);
+ dput(old_parent);
}
+
/*
* "buflen" should be PAGE_SIZE or more.
*/
--- linux-2.1.48/fs/namei.c.old Tue Aug 5 10:46:01 1997
+++ linux-2.1.48/fs/namei.c Sat Aug 9 15:34:25 1997
@@ -234,25 +234,69 @@
*
* We get the directory semaphore, and after getting that we also
* make sure that nobody added the entry to the dcache in the meantime..
+ *
+ * Note that we return the d_mounts dentry with the use count incremented.
*/
static struct dentry * real_lookup(struct dentry * parent, struct qstr * name)
{
- struct dentry * result;
+ struct dentry * result, * new;
struct inode *dir = parent->d_inode;
+ int error;
+
+ /*
+ * Preallocate the new dentry, but defer checking for failure.
+ */
+ new = d_alloc(parent, name);

down(&dir->i_sem);
result = d_lookup(parent, name);
- if (!result) {
- int error;
- result = d_alloc(parent, name);
- error = dir->i_op->lookup(dir, result);
- if (error) {
- d_free(result);
- result = ERR_PTR(error);
- }
- }
+ if (result)
+ goto cached_out;
+
+ result = new;
+ if (!result)
+ goto fail_nomem;
+ error = dir->i_op->lookup(dir, result);
+ if (error)
+ goto bad_lookup;
+#if 1
+ if (list_empty(&result->d_hash))
+ printk("real_lookup: failed to hash dentry\n");
+#endif
+ if (result != result->d_mounts)
+ goto mounted_out;
+
+up_and_out:
up(&dir->i_sem);
return result;
+
+ /*
+ * Looked-up entry had a mount ... fall through to return
+ * the d_mounts dentry and dput() this one.
+ */
+mounted_out:
+ new = result;
+
+ /*
+ * Cached lookup succeeded ... return the d_mounts
+ * dentry and free the extra one.
+ */
+cached_out:
+ result = dget(result->d_mounts);
+ dput(new);
+ goto up_and_out;
+
+ /*
+ * Filesystem lookup failed ... clean up and return error.
+ */
+bad_lookup:
+ dput(result);
+ result = ERR_PTR(error);
+ goto up_and_out;
+
+fail_nomem:
+ result = ERR_PTR(-ENOMEM);
+ goto up_and_out;
}

/*
@@ -320,13 +364,13 @@
if (result)
goto done_noerror;

- result = real_lookup(dir, name);
+ /*
+ * This returns the mounted dentry with the use count incremented.
+ */
+ return real_lookup(dir, name);

- if (!IS_ERR(result)) {
done_noerror:
- result = dget(result->d_mounts);
- }
- return result;
+ return dget(result->d_mounts);
}

static struct dentry * do_follow_link(struct dentry *base, struct dentry *dentry)

--------------9BBC83636660EF090DE14540--