lkml.org 
[lkml]   [2004]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [Patch] cpusets: fix race in cpuset_add_file()
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@bull.net>

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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2005-03-22 14:06    [W:0.050 / U:1.192 seconds]
©2003-2014 Jasper Spaans. Advertise on this site