[BUGFIX][PATCH 0/3] configfs: symlink() fixes

[BUGFIX][PATCH 0/3] configfs: symlink() fixes

[Louis Rilling]

[ 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

[BUGFIX][PATCH 1/3] configfs: Fix symlink() to a removing item

[Louis Rilling]

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@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

[BUGFIX][PATCH 2/3] configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING

[Louis Rilling]

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@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

[BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling]

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@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

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
> 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.

	Oh, no, ugh.  We don't want to create vfs objects first and ask
questions later.  Otherwise we wouldn't need ATTACHING - we'd just
create the symlink, then check dropping.
	If you have ATTACHING set, the rmdir cannot continue - you can
check dropping at that time.  That is, you keep the DROPPING check where
it is - if it is already set, you know that rmdir() is going to complete
successfully.  You can bail before even calling configfs_create_link().
If, however, it isn't set, your ATTACHING protects you from rmdir
throughout.
 
	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 -ENOENT;
			/*
		 	* Force rmdir() of parent_item to wait until we know
		 	* if we succeed.
		 	*/
			parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
		}
		list_add(&sl->sl_list, &target_sd->s_links);
 		spin_unlock(&configfs_dirent_lock);
 		ret = configfs_create_link(sl, parent_item->ci_dentry,
 					   dentry);
		spin_lock(&configfs_dirent_lock);
		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
		if (ret) {
			list_del_init(&sl->sl_list);
 			spin_unlock(&configfs_dirent_lock);
 			config_item_put(item);
 			kfree(sl);
 		} else
			spin_unlock(&configfs_dirent_lock);
 	}
 
 	return ret;

-- 

"When arrows don't penetrate, see...
 Cupid grabs the pistol."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [BUGFIX][PATCH 1/3] configfs: Fix symlink() to a removing item

[Joel Becker]

On Tue, Jun 17, 2008 at 07:37:21PM +0200, Louis Rilling wrote:
> 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.
> 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;

	This needs to be -ENOENT.  If rmdir set DROPPING and then
released dirent_lock, it's going to remove the target.  That's ENOENT.

Joel

-- 

"Sometimes when reading Goethe I have the paralyzing suspicion
 that he is trying to be funny."
         - Guy Davenport

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

On Tue, Jun 17, 2008 at 03:15:28PM -0700, Joel Becker wrote:
> On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
> > 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.
> 
> 	Oh, no, ugh.  We don't want to create vfs objects first and ask
> questions later.  Otherwise we wouldn't need ATTACHING - we'd just
> create the symlink, then check dropping.
> 	If you have ATTACHING set, the rmdir cannot continue - you can
> check dropping at that time.  That is, you keep the DROPPING check where
> it is - if it is already set, you know that rmdir() is going to complete
> successfully.  You can bail before even calling configfs_create_link().
> If, however, it isn't set, your ATTACHING protects you from rmdir
> throughout.

The problem is rmdir() of the target item (see below). ATTACHING only protects
us from rmdir() of the parent. This is the exact reason why I attach the link to
the target in last place, where we know that we won't have to rollback.
	And AFAICS, creating a VFS object can not hurt as long as we hold the
parent i_mutex, right? Otherwise there already is a problem in
configfs_attach_item() where a failure in populate_attrs() leads to rollback the
creation of the VFS object already created for the 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 -ENOENT;
> 			/*
> 		 	* Force rmdir() of parent_item to wait until we know
> 		 	* if we succeed.
> 		 	*/
> 			parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
> 		}
> 		list_add(&sl->sl_list, &target_sd->s_links);
>  		spin_unlock(&configfs_dirent_lock);
>  		ret = configfs_create_link(sl, parent_item->ci_dentry,
>  					   dentry);
> 		spin_lock(&configfs_dirent_lock);
> 		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> 		if (ret) {

Here, if detach_prep() of the target failed because of the link attached above,
it had no means to retry. rmdir() of the target fails because of this
temporary link, which results in a failing symlink() making rmdir() of the
target fail.

> 			list_del_init(&sl->sl_list);
>  			spin_unlock(&configfs_dirent_lock);
>  			config_item_put(item);
>  			kfree(sl);
>  		} else
> 			spin_unlock(&configfs_dirent_lock);
>  	}
>  
>  	return ret;
> 

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
> The problem is rmdir() of the target item (see below). ATTACHING only protects
> us from rmdir() of the parent. This is the exact reason why I attach the link to
> the target in last place, where we know that we won't have to rollback.

	Why wouldn't it protect the target, given that detach_prep()
will be called against the target if it's being rmdir'd?

> 	And AFAICS, creating a VFS object can not hurt as long as we hold the
> parent i_mutex, right? Otherwise there already is a problem in
> configfs_attach_item() where a failure in populate_attrs() leads to rollback the
> creation of the VFS object already created for the item.

	We *can* do that, but we try to isolate it - hand-building VFS
objects is complex and error prone, and I try to isolate that to
specific cases.  I'd rather avoid it when not necessary.

> > 		spin_lock(&configfs_dirent_lock);
> > 		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> > 		if (ret) {
> 
> Here, if detach_prep() of the target failed because of the link attached above,
> it had no means to retry. rmdir() of the target fails because of this
> temporary link, which results in a failing symlink() making rmdir() of the
> target fail.

	How so?  It sees ATTACHING, it gets -EAGAIN, it tries again,
just like before.  What's different?

Joel

-- 

"Anything that is too stupid to be spoken is sung."  
        - Voltaire

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

On Wed, Jun 18, 2008 at 01:11:07PM -0700, Joel Becker wrote:
> On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
> > The problem is rmdir() of the target item (see below). ATTACHING only protects
> > us from rmdir() of the parent. This is the exact reason why I attach the link to
> > the target in last place, where we know that we won't have to rollback.
> 
> 	Why wouldn't it protect the target, given that detach_prep()
> will be called against the target if it's being rmdir'd?

Because
1/ setting and clearing ATTACHING could badly interact with mkdir()/symlink()
inside the target item (for instance clear the flag before mkdir() has finished
attaching a new item); to avoid this we could use a different flag, but
2/ rmdir() of the target cannot lock the inode of the new symlink's parent like
it does for mkdir(), otherwise we would risk a deadlock with other symlink() and
sys_rename(). This means that rmdir() should retry aggressively, in a busy
waiting loop, or replacing mutex_lock()/mutex_unlock() with yield().

> 
> > 	And AFAICS, creating a VFS object can not hurt as long as we hold the
> > parent i_mutex, right? Otherwise there already is a problem in
> > configfs_attach_item() where a failure in populate_attrs() leads to rollback the
> > creation of the VFS object already created for the item.
> 
> 	We *can* do that, but we try to isolate it - hand-building VFS
> objects is complex and error prone, and I try to isolate that to
> specific cases.  I'd rather avoid it when not necessary.

In the case of symlink(), building a new inode is what all filesystems must do.
The only "bad" side-effect I can figure out of having to rollback is that the
new entry will be visible for a short time until it is removed.

Anyway, do you think that the "solutions" above are more acceptable?

> 
> > > 		spin_lock(&configfs_dirent_lock);
> > > 		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> > > 		if (ret) {
> > 
> > Here, if detach_prep() of the target failed because of the link attached above,
> > it had no means to retry. rmdir() of the target fails because of this
> > temporary link, which results in a failing symlink() making rmdir() of the
> > target fail.
> 
> 	How so?  It sees ATTACHING, it gets -EAGAIN, it tries again,
> just like before.  What's different?

See above the reasons for not using ATTACHING on the target.

Louis

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Thu, Jun 19, 2008 at 11:28:42AM +0200, Louis Rilling wrote:
> On Wed, Jun 18, 2008 at 01:11:07PM -0700, Joel Becker wrote:
> > On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
> > > The problem is rmdir() of the target item (see below). ATTACHING only protects
> > > us from rmdir() of the parent. This is the exact reason why I attach the link to
> > > the target in last place, where we know that we won't have to rollback.
> > 
> > 	Why wouldn't it protect the target, given that detach_prep()
> > will be called against the target if it's being rmdir'd?
> 
> Because
> 1/ setting and clearing ATTACHING could badly interact with mkdir()/symlink()
> inside the target item (for instance clear the flag before mkdir() has finished
> attaching a new item); to avoid this we could use a different flag, but

	Ok, true, we don't have a lock to protect mkdir.

> 2/ rmdir() of the target cannot lock the inode of the new symlink's parent like
> it does for mkdir(), otherwise we would risk a deadlock with other symlink() and
> sys_rename(). This means that rmdir() should retry aggressively, in a busy
> waiting loop, or replacing mutex_lock()/mutex_unlock() with yield().

	Yup, we'd have to have some other form of retry - note that this
is all spinlock territory.  Thus, it should be fast.  By the time
rmdir() gets back out to the toplevel, symlink/mkdir should be done
creating whatever they needed and waiting on the dirnet_lock.  Then
rmdir waits again on the lock.  It "should" be bang-bang.
	Yes, I know, assumptions all around.

> > 	We *can* do that, but we try to isolate it - hand-building VFS
> > objects is complex and error prone, and I try to isolate that to
> > specific cases.  I'd rather avoid it when not necessary.
> 
> In the case of symlink(), building a new inode is what all filesystems must do.
> The only "bad" side-effect I can figure out of having to rollback is that the
> new entry will be visible for a short time until it is removed.

	It won't be visible, because we hold i_mutex until we're done.

> Anyway, do you think that the "solutions" above are more acceptable?

	The code for create then destroy was quite ugly.  Maybe it
struck me because of that.

Joel

-- 

 print STDOUT q
 Just another Perl hacker,
 unless $spring
	- Larry Wall

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:

	Ok, I can see some of why I hated this.

> +
>  		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) {

	Use parenthesis.  Actually, separate out the error cases.

> +			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);
> +			}

	This open-code of the VFS munging is ripe to break when the VFS
changes or other configfs changes happen.  The real issue is that you
are reimplementing the core of configfs_unlink().  Note how the core VFS
munging of configfs_rmdir() is separated out so that configfs_mkdir()
can also use it in the failure case?  Do the same with unlink() and it
will read much better ("if (DROPPING) configfs_delete_link()").  Call it
configfs_remove_link() or configfs_delete_link().

Joel

-- 

Life's Little Instruction Book #337

	"Reread your favorite book."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

[PATCH] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

Joel,

What about this other solution? By the way, it does not even need to
rename CONFIGFS_USET_IN_MKDIR.

Louis

-- 
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

[-- Attachment #2: 5d7bd4c6d4b797e1eb9a3f8186338274af50ae4e.diff --]
[-- Type: text/x-patch, Size: 4547 bytes --]

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@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);

Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> What about this other solution? By the way, it does not even need to
> rename CONFIGFS_USET_IN_MKDIR.

	I like it.  My first thought was "why doesn't he take the mutex
during configfs_unlink()?", but the unlink is protected enough by
dirent_lock.

Joel

-- 

"Reality is merely an illusion, albeit a very persistent one."
        - Albert Einstien

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

[Joel Becker]

On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> What about this other solution? By the way, it does not even need to
> rename CONFIGFS_USET_IN_MKDIR.

	We don't need the check for USET_DROPPING either.  I like a lot!

Joel

-- 

"I don't want to achieve immortality through my work; I want to
 achieve immortality through not dying."
        - Woody Allen

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

[Louis Rilling]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On Fri, Jun 20, 2008 at 03:44:20PM -0700, Joel Becker wrote:
> On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
> > What about this other solution? By the way, it does not even need to
> > rename CONFIGFS_USET_IN_MKDIR.
> 
> 	We don't need the check for USET_DROPPING either.  I like a lot!

I hope that you wasn't speaking about checking USET_DROPPING on the target. We
still need to check it, of course. I'm resending the updated, hopefully final,
patchset.

Louis

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]