Louis Rilling
2008-Jun-17 17:37 UTC
[Ocfs2-devel] [BUGFIX][PATCH 0/3] configfs: symlink() fixes
[ applies on top of the previously submitted rename() vs rmdir() deadlock fix ] Hi, The following patchset fixes incorrect symlinks to dead items in configfs, which are forbidden by specification. The first patch actually prevents such dangling symlinks from being created, but introduces a weird(?) behavior where a failing symlink() can make a racing rmdir() fail in the symlink's parent and in the symlink's target as well. The next patches prevent this behavior using a similar idea as for the mkdir() vs rmdir() case previously submitted. Summary: configfs: Fix symlink() to a removing item configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING configfs: Fix failing symlink() making rmdir() fail fs/configfs/configfs_internal.h | 2 +- fs/configfs/dir.c | 20 ++++++++++---------- fs/configfs/symlink.c | 33 +++++++++++++++++++++++++++++---- 3 files changed, 40 insertions(+), 15 deletions(-) -- 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
Louis Rilling
2008-Jun-17 17:37 UTC
[Ocfs2-devel] [BUGFIX][PATCH 1/3] configfs: Fix symlink() to a removing item
The rule for configfs symlinks is that symlinks always point to valid
config_items, and prevent the target from being removed. However,
configfs_symlink() only checks that it can grab a reference on the target item,
without ensuring that it remains alive until the symlink is correctly attached.
This patch makes configfs_symlink() fail whenever the target is being removed,
using the CONFIGFS_USET_DROPPING flag set by configfs_detach_prep() and
protected by configfs_dirent_lock.
This patch introduces a similar (weird?) behavior as with mkdir failures making
rmdir fail: if symlink() races with rmdir() of the parent directory (or its
youngest user-created ancestor if parent is a default group) or rmdir() of the
target directory, and then fails in configfs_create(), this can make the racing
rmdir() fail despite the concerned directory having no user-created entry (resp.
no symlink pointing to it or one of its default groups) in the end.
This behavior is fixed in later patches.
Signed-off-by: Louis Rilling <louis.rilling at kerlabs.com>
---
fs/configfs/dir.c | 14 +++++++-------
fs/configfs/symlink.c | 6 ++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 614e382..f2a12d0 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -370,6 +370,9 @@ static int configfs_detach_prep(struct dentry *dentry,
struct mutex **wait_mutex
struct configfs_dirent *sd;
int ret;
+ /* Mark that we're trying to drop the group */
+ parent_sd->s_type |= CONFIGFS_USET_DROPPING;
+
ret = -EBUSY;
if (!list_empty(&parent_sd->s_links))
goto out;
@@ -385,8 +388,6 @@ static int configfs_detach_prep(struct dentry *dentry,
struct mutex **wait_mutex
*wait_mutex = &sd->s_dentry->d_inode->i_mutex;
return -EAGAIN;
}
- /* Mark that we're trying to drop the group */
- sd->s_type |= CONFIGFS_USET_DROPPING;
/*
* Yup, recursive. If there's a problem, blame
@@ -414,12 +415,11 @@ static void configfs_detach_rollback(struct dentry
*dentry)
struct configfs_dirent *parent_sd = dentry->d_fsdata;
struct configfs_dirent *sd;
- list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
- if (sd->s_type & CONFIGFS_USET_DEFAULT) {
+ parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
+
+ list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
+ if (sd->s_type & CONFIGFS_USET_DEFAULT)
configfs_detach_rollback(sd->s_dentry);
- sd->s_type &= ~CONFIGFS_USET_DROPPING;
- }
- }
}
static void detach_attrs(struct config_item * item)
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index faeb441..58722a9 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -78,6 +78,12 @@ static int create_link(struct config_item *parent_item,
if (sl) {
sl->sl_target = config_item_get(item);
spin_lock(&configfs_dirent_lock);
+ if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
+ spin_unlock(&configfs_dirent_lock);
+ config_item_put(item);
+ kfree(sl);
+ return -EPERM;
+ }
list_add(&sl->sl_list, &target_sd->s_links);
spin_unlock(&configfs_dirent_lock);
ret = configfs_create_link(sl, parent_item->ci_dentry,
--
1.5.5.3
Louis Rilling
2008-Jun-17 17:37 UTC
[Ocfs2-devel] [BUGFIX][PATCH 2/3] configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING
The CONFIGFS_USET_IN_MKDIR flag can be reused with symlink() to solve a similar
issue as mkdir() vs rmdir(). This patch renames the flag to make it more
meaningful for this purpose.
Signed-off-by: Louis Rilling <louis.rilling at kerlabs.com>
---
fs/configfs/configfs_internal.h | 2 +-
fs/configfs/dir.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index da015c1..a28f37d 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -48,7 +48,7 @@ struct configfs_dirent {
#define CONFIGFS_USET_DIR 0x0040
#define CONFIGFS_USET_DEFAULT 0x0080
#define CONFIGFS_USET_DROPPING 0x0100
-#define CONFIGFS_USET_IN_MKDIR 0x0200
+#define CONFIGFS_USET_ATTACHING 0x0200
#define CONFIGFS_NOT_PINNED (CONFIGFS_ITEM_ATTR)
extern spinlock_t configfs_dirent_lock;
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index f2a12d0..629c938 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -383,7 +383,7 @@ static int configfs_detach_prep(struct dentry *dentry,
struct mutex **wait_mutex
continue;
if (sd->s_type & CONFIGFS_USET_DEFAULT) {
/* Abort if racing with mkdir() */
- if (sd->s_type & CONFIGFS_USET_IN_MKDIR) {
+ if (sd->s_type & CONFIGFS_USET_ATTACHING) {
if (wait_mutex)
*wait_mutex = &sd->s_dentry->d_inode->i_mutex;
return -EAGAIN;
@@ -1127,7 +1127,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry
*dentry, int mode)
*/
spin_lock(&configfs_dirent_lock);
/* This will make configfs_detach_prep() fail */
- sd->s_type |= CONFIGFS_USET_IN_MKDIR;
+ sd->s_type |= CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
if (group)
@@ -1136,7 +1136,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry
*dentry, int mode)
ret = configfs_attach_item(parent_item, item, dentry);
spin_lock(&configfs_dirent_lock);
- sd->s_type &= ~CONFIGFS_USET_IN_MKDIR;
+ sd->s_type &= ~CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
out_unlink:
--
1.5.5.3
Louis Rilling
2008-Jun-17 17:37 UTC
[Ocfs2-devel] [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail
On a similar pattern as mkdir() vs rmdir(), a failing symlink() may make rmdir()
fail for the symlink's parent and the symlink's target as well.
failing symlink() making target's rmdir() fail:
process 1: process 2:
symlink("A/S" -> "B")
allow_link()
create_link()
attach to "B" links list
rmdir("B")
detach_prep("B")
error because of new link
configfs_create_link("A", "S")
error (eg -ENOMEM)
failing symlink() making parent's rmdir() fail:
process 1: process 2:
symlink("A/D/S" -> "B")
allow_link()
create_link()
attach to "B" links list
configfs_create_link("A/D", "S")
make_dirent("A/D", "S")
rmdir("A")
detach_prep("A")
detach_prep("A/D")
error because of "S"
create("S")
error (eg -ENOMEM)
For the parent's rmdir() case, we can use the same solution as with mkdir()
vs
rmdir(). For the target's rmdir() case, we cannot, since we do not and
cannot
lock the target's inode while in symlink(). Fortunately, once create_link()
terminates, no further operation can fail in symlink(). So we first reorder the
operations in create_link() to attach the new symlink to its target in last
place, and second handle symlink creation failure the same way as a new item
creation failure.
Signed-off-by: Louis Rilling <louis.rilling at kerlabs.com>
---
fs/configfs/symlink.c | 39 +++++++++++++++++++++++++++++----------
1 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 58722a9..0c5e650 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -69,6 +69,7 @@ static int create_link(struct config_item *parent_item,
struct config_item *item,
struct dentry *dentry)
{
+ struct configfs_dirent *parent_sd = parent_item->ci_dentry->d_fsdata;
struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
struct configfs_symlink *sl;
int ret;
@@ -77,24 +78,42 @@ static int create_link(struct config_item *parent_item,
sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
if (sl) {
sl->sl_target = config_item_get(item);
+
spin_lock(&configfs_dirent_lock);
- if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
- spin_unlock(&configfs_dirent_lock);
- config_item_put(item);
- kfree(sl);
- return -EPERM;
- }
- list_add(&sl->sl_list, &target_sd->s_links);
+ /*
+ * Force rmdir() of parent_item to wait until we know if we
+ * succeed.
+ */
+ parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
spin_unlock(&configfs_dirent_lock);
+
ret = configfs_create_link(sl, parent_item->ci_dentry,
dentry);
- if (ret) {
- spin_lock(&configfs_dirent_lock);
- list_del_init(&sl->sl_list);
+
+ spin_lock(&configfs_dirent_lock);
+ parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
+
+ if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {
+ struct configfs_dirent *sd = NULL;
+
+ if (!ret) {
+ sd = dentry->d_fsdata;
+ list_del_init(&sd->s_sibling);
+ }
spin_unlock(&configfs_dirent_lock);
+
+ if (!ret) {
+ configfs_drop_dentry(sd, dentry->d_parent);
+ d_delete(dentry);
+ configfs_put(sd);
+ }
config_item_put(item);
kfree(sl);
+ return ret ? ret : -EPERM;
}
+
+ list_add(&sl->sl_list, &target_sd->s_links);
+ spin_unlock(&configfs_dirent_lock);
}
return ret;
--
1.5.5.3