From: Louis Rilling <Louis.Rilling@kerlabs.com>
To: Joel.Becker@oracle.com
Cc: Louis.Rilling@kerlabs.com, linux-kernel@vger.kernel.org,
ocfs2-devel@oss.oracle.com
Subject: [Ocfs2-devel] [PATCH 3/3][BUGFIX] configfs: Fix deadlock with racing rmdir() and rename()
Date: Thu, 12 Jun 2008 15:31:29 +0200 [thread overview]
Message-ID: <20080612134204.097928479@kerlabs.com> (raw)
In-Reply-To: 20080612133126.335618468@kerlabs.com
An embedded and charset-unspecified text was scrubbed...
Name: configfs-fix-rmdir-vs-rename-deadlock.patch
Url: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080612/4c8f4ea8/attachment.pl
WARNING: multiple messages have this Message-ID (diff)
From: Louis Rilling <Louis.Rilling@kerlabs.com>
To: Joel.Becker@oracle.com
Cc: Louis.Rilling@kerlabs.com, linux-kernel@vger.kernel.org,
ocfs2-devel@oss.oracle.com
Subject: [PATCH 3/3][BUGFIX] configfs: Fix deadlock with racing rmdir() and rename()
Date: Thu, 12 Jun 2008 15:31:29 +0200 [thread overview]
Message-ID: <20080612134204.097928479@kerlabs.com> (raw)
In-Reply-To: 20080612133126.335618468@kerlabs.com
[-- Attachment #1: configfs-fix-rmdir-vs-rename-deadlock.patch --]
[-- Type: text/plain, Size: 5737 bytes --]
This patch fixes the deadlock between racing sys_rename() and configfs_rmdir().
The idea is to avoid locking i_mutexes of default groups in
configfs_detach_prep(), and rely instead on the new configfs_dirent_lock to
protect against configfs_dirent's linkage mutations. To ensure that an mkdir()
racing with rmdir() will not create new items in a to-be-removed default group,
we make configfs_new_dirent() check for the CONFIGFS_USET_DROPPING flag right
before linking the new dirent, and return error if the flag is set. This makes
racing mkdir()/symlink()/dir_open() fail in places where errors could already
happen, resp. in (attach_item()|attach_group())/create_link()/new_dirent().
configfs_depend() remains safe since it locks all the path from configfs root,
and is thus mutually exclusive with rmdir().
An advantage of this is that now detach_groups() unconditionnaly takes the
default groups i_mutex, which makes it more consistent with populate_groups().
Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com>
---
fs/configfs/dir.c | 44 ++++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c 2008-06-12 13:45:18.000000000 +0200
+++ b/fs/configfs/dir.c 2008-06-12 13:50:10.000000000 +0200
@@ -38,6 +38,9 @@
DECLARE_RWSEM(configfs_rename_sem);
/*
* Protects configfs_dirent traversals against linkage mutations
+ * Protects setting of CONFIGFS_USET_DROPPING: checking the flag
+ * unlocked is not reliable unless in detach_groups called from
+ * rmdir/unregister and from configfs_attach_group
* Can be used as an alternative to taking the concerned i_mutex
*/
DEFINE_SPINLOCK(configfs_dirent_lock);
@@ -86,6 +89,11 @@ static struct configfs_dirent *configfs_
INIT_LIST_HEAD(&sd->s_links);
INIT_LIST_HEAD(&sd->s_children);
spin_lock(&configfs_dirent_lock);
+ if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
+ spin_unlock(&configfs_dirent_lock);
+ kmem_cache_free(configfs_dir_cachep, sd);
+ return ERR_PTR(-ENOENT);
+ }
list_add(&sd->s_sibling, &parent_sd->s_children);
spin_unlock(&configfs_dirent_lock);
sd->s_element = element;
@@ -345,11 +353,11 @@ static struct dentry * configfs_lookup(s
/*
* Only subdirectories count here. Files (CONFIGFS_NOT_PINNED) are
- * attributes and are removed by rmdir(). We recurse, taking i_mutex
- * on all children that are candidates for default detach. If the
- * result is clean, then configfs_detach_group() will handle dropping
- * i_mutex. If there is an error, the caller will clean up the i_mutex
- * holders via configfs_detach_rollback().
+ * attributes and are removed by rmdir(). We recurse, setting
+ * CONFIGFS_USET_DROPPING on all children that are candidates for
+ * default detach.
+ * If there is an error, the caller will reset the flags via
+ * configfs_detach_rollback().
*/
static int configfs_detach_prep(struct dentry *dentry)
{
@@ -366,8 +374,7 @@ static int configfs_detach_prep(struct d
if (sd->s_type & CONFIGFS_NOT_PINNED)
continue;
if (sd->s_type & CONFIGFS_USET_DEFAULT) {
- mutex_lock(&sd->s_dentry->d_inode->i_mutex);
- /* Mark that we've taken i_mutex */
+ /* Mark that we're trying to drop the group */
sd->s_type |= CONFIGFS_USET_DROPPING;
/*
@@ -388,7 +395,7 @@ out:
}
/*
- * Walk the tree, dropping i_mutex wherever CONFIGFS_USET_DROPPING is
+ * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
* set.
*/
static void configfs_detach_rollback(struct dentry *dentry)
@@ -399,11 +406,7 @@ static void configfs_detach_rollback(str
list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
if (sd->s_type & CONFIGFS_USET_DEFAULT) {
configfs_detach_rollback(sd->s_dentry);
-
- if (sd->s_type & CONFIGFS_USET_DROPPING) {
- sd->s_type &= ~CONFIGFS_USET_DROPPING;
- mutex_unlock(&sd->s_dentry->d_inode->i_mutex);
- }
+ sd->s_type &= ~CONFIGFS_USET_DROPPING;
}
}
}
@@ -482,16 +485,12 @@ static void detach_groups(struct config_
child = sd->s_dentry;
+ mutex_lock(&child->d_inode->i_mutex);
+
configfs_detach_group(sd->s_element);
child->d_inode->i_flags |= S_DEAD;
- /*
- * From rmdir/unregister, a configfs_detach_prep() pass
- * has taken our i_mutex for us. Drop it.
- * From mkdir/register cleanup, there is no sem held.
- */
- if (sd->s_type & CONFIGFS_USET_DROPPING)
- mutex_unlock(&child->d_inode->i_mutex);
+ mutex_unlock(&child->d_inode->i_mutex);
d_delete(child);
dput(child);
@@ -1177,12 +1176,15 @@ static int configfs_rmdir(struct inode *
return -EINVAL;
}
+ spin_lock(&configfs_dirent_lock);
ret = configfs_detach_prep(dentry);
if (ret) {
configfs_detach_rollback(dentry);
+ spin_unlock(&configfs_dirent_lock);
config_item_put(parent_item);
return ret;
}
+ spin_unlock(&configfs_dirent_lock);
/* Get a working ref for the duration of this function */
item = configfs_get_config_item(dentry);
@@ -1474,9 +1476,11 @@ void configfs_unregister_subsystem(struc
mutex_lock_nested(&configfs_sb->s_root->d_inode->i_mutex,
I_MUTEX_PARENT);
mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD);
+ spin_lock(&configfs_dirent_lock);
if (configfs_detach_prep(dentry)) {
printk(KERN_ERR "configfs: Tried to unregister non-empty subsystem!\n");
}
+ spin_unlock(&configfs_dirent_lock);
configfs_detach_group(&group->cg_item);
dentry->d_inode->i_flags |= S_DEAD;
mutex_unlock(&dentry->d_inode->i_mutex);
--
Dr Louis Rilling Kerlabs
Skype: louis.rilling Batiment Germanium
Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
http://www.kerlabs.com/ 35700 Rennes
next prev parent reply other threads:[~2008-06-12 13:31 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-12 13:31 [Ocfs2-devel] [PATCH 0/3][BUGFIX] configfs: Fix deadlock rmdir() vs rename() Louis Rilling
2008-06-12 13:31 ` Louis Rilling
2008-06-12 13:31 ` [Ocfs2-devel] [PATCH 1/3][BUGFIX] configfs: Introduce configfs_dirent_lock Louis Rilling
2008-06-12 13:31 ` Louis Rilling
2008-06-12 19:13 ` [Ocfs2-devel] " Joel Becker
2008-06-12 19:13 ` Joel Becker
2008-06-12 22:25 ` [Ocfs2-devel] " Louis Rilling
2008-06-12 22:25 ` Louis Rilling
2008-06-13 2:41 ` [Ocfs2-devel] " Joel Becker
2008-06-13 2:41 ` Joel Becker
2008-06-13 10:45 ` [Ocfs2-devel] " Louis Rilling
2008-06-13 10:45 ` Louis Rilling
2008-06-13 12:09 ` [Ocfs2-devel] " Louis Rilling
2008-06-13 12:09 ` Louis Rilling
2008-06-13 20:19 ` [Ocfs2-devel] " Joel Becker
2008-06-13 20:19 ` Joel Becker
2008-06-13 20:17 ` [Ocfs2-devel] " Joel Becker
2008-06-13 20:17 ` Joel Becker
2008-06-13 21:54 ` [Ocfs2-devel] " Louis Rilling
2008-06-13 21:54 ` Louis Rilling
2008-06-13 22:34 ` [Ocfs2-devel] " Joel Becker
2008-06-13 22:34 ` Joel Becker
2008-06-16 11:30 ` [Ocfs2-devel] " Louis Rilling
2008-06-16 11:30 ` Louis Rilling
2008-06-12 13:31 ` [Ocfs2-devel] [PATCH 2/3][BUGFIX] configfs: Make configfs_new_dirent() return error code instead of NULL Louis Rilling
2008-06-12 13:31 ` Louis Rilling
2008-06-12 13:31 ` Louis Rilling [this message]
2008-06-12 13:31 ` [PATCH 3/3][BUGFIX] configfs: Fix deadlock with racing rmdir() and rename() Louis Rilling
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080612134204.097928479@kerlabs.com \
--to=louis.rilling@kerlabs.com \
--cc=Joel.Becker@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ocfs2-devel@oss.oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.