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

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

[Louis Rilling]

[-- Attachment #1.1: Type: text/plain, Size: 292 bytes --]

Hi, the following patch fixes the symlink bug I mentioned a few days ago.
Thanks for your comments.

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 #1.2: configfs-do-not-symlink-to-removing-item.patch --]
[-- Type: text/x-diff, Size: 3470 bytes --]

configfs: Fix symlink() to a removing item

[Applies on top of rename() vs rmdir() deadlock fix patchset]

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.
If this behavior is found unacceptable, I'll submit a fix in the same spirit as
the racing mkdir() fix.

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

Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c	2008-06-16 19:35:57.000000000 +0200
+++ b/fs/configfs/dir.c	2008-06-16 19:38:47.000000000 +0200
@@ -370,6 +370,9 @@ static int configfs_detach_prep(struct d
 	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 d
 					*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(str
 	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)
Index: b/fs/configfs/symlink.c
===================================================================
--- a/fs/configfs/symlink.c	2008-06-16 19:43:34.000000000 +0200
+++ b/fs/configfs/symlink.c	2008-06-16 19:47:06.000000000 +0200
@@ -78,6 +78,12 @@ static int create_link(struct config_ite
 	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,

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

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

[Joel Becker]

On Mon, Jun 16, 2008 at 08:09:11PM +0200, Louis Rilling wrote:
> 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.
> If this behavior is found unacceptable, I'll submit a fix in the same spirit as
> the racing mkdir() fix.

	Ahh, but you can't wait on the mutex like you do in mkdir(),
because they're not ordered - it can race rename again.

Joel
 

-- 

Dort wo man Bücher verbrennt, verbrennt man am Ende auch Mensch.
(Wherever they burn books, they will also end up burning people.)
	- Heinrich Heine

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

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

[Louis Rilling]

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

On Mon, Jun 16, 2008 at 03:29:00PM -0700, Joel Becker wrote:
> On Mon, Jun 16, 2008 at 08:09:11PM +0200, Louis Rilling wrote:
> > 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.
> > If this behavior is found unacceptable, I'll submit a fix in the same spirit as
> > the racing mkdir() fix.
> 
> 	Ahh, but you can't wait on the mutex like you do in mkdir(),
> because they're not ordered - it can race rename again.

Yes I can. In the mkdir() case, rmdir() waits on the mutex of one of the default
groups. The symlink case will be handled the same. The difference is that while
mkdir() tags the parent with CONFIGFS_USET_IN_MKDIR, symlink() will tag both the
parent and the target with CONFIGFS_USET_IN_SYMLINK. [ We can actually merge the
two flags in something like CONFIGFS_USET_ATTACHING. ] Since tagging requires
only to take configfs_dirent_lock, there will be no deadlock.

I'll send a patch later.

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: [PATCH][BUGFIX] configfs: Fix symlink() to a removing item

[Louis Rilling]

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

On Tue, Jun 17, 2008 at 12:42:59PM +0200, Louis Rilling wrote:
> On Mon, Jun 16, 2008 at 03:29:00PM -0700, Joel Becker wrote:
> > On Mon, Jun 16, 2008 at 08:09:11PM +0200, Louis Rilling wrote:
> > > 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.
> > > If this behavior is found unacceptable, I'll submit a fix in the same spirit as
> > > the racing mkdir() fix.
> > 
> > 	Ahh, but you can't wait on the mutex like you do in mkdir(),
> > because they're not ordered - it can race rename again.
> 
> Yes I can. In the mkdir() case, rmdir() waits on the mutex of one of the default
> groups. The symlink case will be handled the same. The difference is that while
> mkdir() tags the parent with CONFIGFS_USET_IN_MKDIR, symlink() will tag both the
> parent and the target with CONFIGFS_USET_IN_SYMLINK. [ We can actually merge the
> two flags in something like CONFIGFS_USET_ATTACHING. ] Since tagging requires
> only to take configfs_dirent_lock, there will be no deadlock.

Hm, you were right, sorry. We can only solve the race with rmdir() removing the
item where the symlink is created. For the target item, I don't know yet...

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