[ 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