[ 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. This
behavior is fixed with the next patch.
Changelog:
- fix error code when symlink's target is being removed
- re-implemented the weird(?) behavior fix in a way that does not temporarily
instantiate the new symlink in the VFS.
Summary:
configfs: Fix symlink() to a removing item
configfs: Fix failing symlink() making rmdir() fail
fs/configfs/configfs_internal.h | 1 +
fs/configfs/dir.c | 24 +++++++++++++++++-------
fs/configfs/symlink.c | 14 +++++++++++++-
3 files changed, 31 insertions(+), 8 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-23 12:16 UTC
[Ocfs2-devel] [PATCH 1/2] 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..b66908d 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 -ENOENT;
+ }
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-23 12:16 UTC
[Ocfs2-devel] [PATCH 2/2] 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)
We cannot use the same solution as for mkdir() vs rmdir(), since rmdir() on the
target cannot wait on the i_mutex of the new symlink's parent without
risking a
deadlock (with other symlink() or sys_rename()). Instead we define a global
mutex protecting all configfs symlinks attachment, so that rmdir() can avoid the
races above.
Signed-off-by: Louis Rilling <louis.rilling at kerlabs.com>
---
fs/configfs/configfs_internal.h | 1 +
fs/configfs/dir.c | 10 ++++++++++
fs/configfs/symlink.c | 8 +++++++-
3 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index da015c1..5f61b26 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -51,6 +51,7 @@ struct configfs_dirent {
#define CONFIGFS_USET_IN_MKDIR 0x0200
#define CONFIGFS_NOT_PINNED (CONFIGFS_ITEM_ATTR)
+extern struct mutex configfs_symlink_mutex;
extern spinlock_t configfs_dirent_lock;
extern struct vfsmount * configfs_mount;
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index f2a12d0..2c873fd 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1202,6 +1202,11 @@ static int configfs_rmdir(struct inode *dir, struct
dentry *dentry)
return -EINVAL;
}
+ /*
+ * Ensure that no racing symlink() will make detach_prep() fail while
+ * the new link is temporarily attached
+ */
+ mutex_lock(&configfs_symlink_mutex);
spin_lock(&configfs_dirent_lock);
do {
struct mutex *wait_mutex;
@@ -1210,6 +1215,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry
*dentry)
if (ret) {
configfs_detach_rollback(dentry);
spin_unlock(&configfs_dirent_lock);
+ mutex_unlock(&configfs_symlink_mutex);
if (ret != -EAGAIN) {
config_item_put(parent_item);
return ret;
@@ -1219,10 +1225,12 @@ static int configfs_rmdir(struct inode *dir, struct
dentry *dentry)
mutex_lock(wait_mutex);
mutex_unlock(wait_mutex);
+ mutex_lock(&configfs_symlink_mutex);
spin_lock(&configfs_dirent_lock);
}
} while (ret == -EAGAIN);
spin_unlock(&configfs_dirent_lock);
+ mutex_unlock(&configfs_symlink_mutex);
/* Get a working ref for the duration of this function */
item = configfs_get_config_item(dentry);
@@ -1512,11 +1520,13 @@ void configfs_unregister_subsystem(struct
configfs_subsystem *subsys)
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);
+ mutex_lock(&configfs_symlink_mutex);
spin_lock(&configfs_dirent_lock);
if (configfs_detach_prep(dentry, NULL)) {
printk(KERN_ERR "configfs: Tried to unregister non-empty
subsystem!\n");
}
spin_unlock(&configfs_dirent_lock);
+ mutex_unlock(&configfs_symlink_mutex);
configfs_detach_group(&group->cg_item);
dentry->d_inode->i_flags |= S_DEAD;
mutex_unlock(&dentry->d_inode->i_mutex);
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index b66908d..21bd751 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -31,6 +31,9 @@
#include <linux/configfs.h>
#include "configfs_internal.h"
+/* Protects attachments of new symlinks */
+DEFINE_MUTEX(configfs_symlink_mutex);
+
static int item_depth(struct config_item * item)
{
struct config_item * p = item;
@@ -146,8 +149,11 @@ int configfs_symlink(struct inode *dir, struct dentry
*dentry, const char *symna
goto out_put;
ret = type->ct_item_ops->allow_link(parent_item, target_item);
- if (!ret)
+ if (!ret) {
+ mutex_lock(&configfs_symlink_mutex);
ret = create_link(parent_item, target_item, dentry);
+ mutex_unlock(&configfs_symlink_mutex);
+ }
config_item_put(target_item);
path_put(&nd.path);
--
1.5.5.3
On Mon, Jun 23, 2008 at 02:16:16PM +0200, Louis Rilling wrote:> 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. This > behavior is fixed with the next patch.Silly question: you've tested this, right? Joel -- "I almost ran over an angel He had a nice big fat cigar. 'In a sense,' he said, 'You're alone here So if you jump, you'd best jump far.'" Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127