Re: [Patch] cpusets: fix race in cpuset_add_file()

From: Simon Derr
Date: Thu Sep 16 2004 - 10:47:10 EST


On Thu, 16 Sep 2004, Paul Jackson wrote:

Color me confused - this cpuset_sem down/up should not be needed,
and should deadlock. In the call chain:

cpuset_mkdir -> cpuset_create -> cpuset_populate_dir -> cpuset_add_file

cpuset_create() already holds the cpuset_sem for the duration, and you're
adding another cpuset_sem down in cpuset_add_file(), which should deadlock.

If you are seeing the duplicate invalid cpuset entries, then must be
something else going on, unfortunately.

That, or I'm confused.
Neither.
You've just read the patch too quickly:

+ down(&dir->d_inode->i_sem);

NOT down(&cpuset_sem);

However, your remark is welcome, since there is indeed a slight chance of deadlock with my patch, but it needs 2 mkdirs racing.

imagine:

mkdir a/b mkdir a/b/c

sys_mkdir(): down(a->i_sem);
cpuset_create(): down(cpuset_sem);
sys_mkdir(): down(b->i_sem);
cpuset_add_file(): down(b->i_sem);
cpuset_create(): down(cpuset_sem);


-> deadlock.


So we should release cpuset_sem a bit earlier in cpuset_create(), before calling cpuset_populate_dir().





This updated patch should be better (hopefully).

Signed-off-by: Simon Derr <simon.derr@xxxxxxxx>

Index: mm4/kernel/cpuset.c
===================================================================
--- mm4.orig/kernel/cpuset.c 2004-09-13 09:43:02.000000000 +0200
+++ mm4/kernel/cpuset.c 2004-09-16 17:23:48.764321923 +0200
@@ -956,13 +956,12 @@ static int cpuset_create_dir(struct cpus
return error;
}

-/* MUST be called with dir->d_inode->i_sem held */
-
static int cpuset_add_file(struct dentry *dir, const struct cftype *cft)
{
struct dentry *dentry;
int error;

+ down(&dir->d_inode->i_sem);
dentry = cpuset_get_dentry(dir, cft->name);
if (!IS_ERR(dentry)) {
error = cpuset_create_file(dentry, 0644 | S_IFREG);
@@ -971,6 +970,7 @@ static int cpuset_add_file(struct dentry
dput(dentry);
} else
error = PTR_ERR(dentry);
+ up(&dir->d_inode->i_sem);
return error;
}

@@ -1162,7 +1162,6 @@ static struct cftype cft_notify_on_relea
.private = FILE_NOTIFY_ON_RELEASE,
};

-/* MUST be called with ->d_inode->i_sem held */
static int cpuset_populate_dir(struct dentry *cs_dentry)
{
int err;
@@ -1219,9 +1218,16 @@ static long cpuset_create(struct cpuset
err = cpuset_create_dir(cs, name, mode);
if (err < 0)
goto err;
+
+ /* release cpuset_sem before cpuset_populate_dir()
+ * because it will down() this new directory's i_sem
+ * and if we race with another mkdir,
+ * we might deadlock + */
+ up(&cpuset_sem);
+
err = cpuset_populate_dir(cs->dentry);
/* If err < 0, we have a half-filled directory - oh well ;) */
- up(&cpuset_sem);
return 0;
err:
list_del(&cs->sibling);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/