configfs: Fix deadlock with racing rmdir() and rename()

This patch fixes the deadlock between racing sys_rename() and configfs_rmdir(). The idea is to avoid locking i_mutexes of default groups in configfs_detach_prep(), and rely instead on the new configfs_dirent_lock to protect against configfs_dirent's linkage mutations. To ensure that an mkdir() racing with rmdir() will not create new items in a to-be-removed default group, we make configfs_new_dirent() check for the CONFIGFS_USET_DROPPING flag right before linking the new dirent, and return error if the flag is set. This makes racing mkdir()/symlink()/dir_open() fail in places where errors could already happen, resp. in (attach_item()|attach_group())/create_link()/new_dirent(). configfs_depend() remains safe since it locks all the path from configfs root, and is thus mutually exclusive with rmdir(). An advantage of this is that now detach_groups() unconditionnaly takes the default groups i_mutex, which makes it more consistent with populate_groups(). Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com> Signed-off-by: Joel Becker <joel.becker@oracle.com>

configfs: Fix deadlock with racing rmdir() and rename()
This patch fixes the deadlock between racing sys_rename() and configfs_rmdir(). The idea is to avoid locking i_mutexes of default groups in configfs_detach_prep(), and rely instead on the new configfs_dirent_lock to protect against configfs_dirent's linkage mutations. To ensure that an mkdir() racing with rmdir() will not create new items in a to-be-removed default group, we make configfs_new_dirent() check for the CONFIGFS_USET_DROPPING flag right before linking the new dirent, and return error if the flag is set. This makes racing mkdir()/symlink()/dir_open() fail in places where errors could already happen, resp. in (attach_item()|attach_group())/create_link()/new_dirent(). configfs_depend() remains safe since it locks all the path from configfs root, and is thus mutually exclusive with rmdir(). An advantage of this is that now detach_groups() unconditionnaly takes the default groups i_mutex, which makes it more consistent with populate_groups(). Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com> Signed-off-by: Joel Becker <joel.becker@oracle.com>
Louis Rilling · Mark Fasheh
1 parent 107ed40bd0
Showing 1 changed file with 25 additions and 20 deletions Side-by-side Diff
fs/configfs/dir.c
@@ -43,6 +43,10 @@
  * and configfs_dirent_lock locked, in that order.
  * This allows one to safely traverse configfs_dirent trees and symlinks without
  * having to lock inodes.
+ *
+ * Protects setting of CONFIGFS_USET_DROPPING: checking the flag
+ * unlocked is not reliable unless in detach_groups() called from
+ * rmdir()/unregister() and from configfs_attach_group()
  */
 DEFINE_SPINLOCK(configfs_dirent_lock);
  
@@ -91,6 +95,11 @@
 	INIT_LIST_HEAD(&sd->s_children);
 	sd->s_element = element;
 	spin_lock(&configfs_dirent_lock);
+	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
+		spin_unlock(&configfs_dirent_lock);
+		kmem_cache_free(configfs_dir_cachep, sd);
+		return ERR_PTR(-ENOENT);
+	}
 	list_add(&sd->s_sibling, &parent_sd->s_children);
 	spin_unlock(&configfs_dirent_lock);
  
@@ -349,11 +358,11 @@
  
 /*
  * Only subdirectories count here.  Files (CONFIGFS_NOT_PINNED) are
- * attributes and are removed by rmdir().  We recurse, taking i_mutex
- * on all children that are candidates for default detach.  If the
- * result is clean, then configfs_detach_group() will handle dropping
- * i_mutex.  If there is an error, the caller will clean up the i_mutex
- * holders via configfs_detach_rollback().
+ * attributes and are removed by rmdir().  We recurse, setting
+ * CONFIGFS_USET_DROPPING on all children that are candidates for
+ * default detach.
+ * If there is an error, the caller will reset the flags via
+ * configfs_detach_rollback().
  */
 static int configfs_detach_prep(struct dentry *dentry)
 {
@@ -370,8 +379,7 @@
 		if (sd->s_type & CONFIGFS_NOT_PINNED)
 			continue;
 		if (sd->s_type & CONFIGFS_USET_DEFAULT) {
-			mutex_lock(&sd->s_dentry->d_inode->i_mutex);
-			/* Mark that we've taken i_mutex */
+			/* Mark that we're trying to drop the group */
 			sd->s_type |= CONFIGFS_USET_DROPPING;
  
 			/*
@@ -392,7 +400,7 @@
 }
  
 /*
- * Walk the tree, dropping i_mutex wherever CONFIGFS_USET_DROPPING is
+ * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
  * set.
  */
 static void configfs_detach_rollback(struct dentry *dentry)
@@ -403,11 +411,7 @@
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
 		if (sd->s_type & CONFIGFS_USET_DEFAULT) {
 			configfs_detach_rollback(sd->s_dentry);
-
-			if (sd->s_type & CONFIGFS_USET_DROPPING) {
-				sd->s_type &= ~CONFIGFS_USET_DROPPING;
-				mutex_unlock(&sd->s_dentry->d_inode->i_mutex);
-			}
+			sd->s_type &= ~CONFIGFS_USET_DROPPING;
 		}
 	}
 }
  
@@ -486,16 +490,12 @@
  
 		child = sd->s_dentry;
  
+		mutex_lock(&child->d_inode->i_mutex);
+
 		configfs_detach_group(sd->s_element);
 		child->d_inode->i_flags |= S_DEAD;
  
-		/*
-		 * From rmdir/unregister, a configfs_detach_prep() pass
-		 * has taken our i_mutex for us.  Drop it.
-		 * From mkdir/register cleanup, there is no sem held.
-		 */
-		if (sd->s_type & CONFIGFS_USET_DROPPING)
-			mutex_unlock(&child->d_inode->i_mutex);
+		mutex_unlock(&child->d_inode->i_mutex);
  
 		d_delete(child);
 		dput(child);
  
  
@@ -1181,12 +1181,15 @@
 		return -EINVAL;
 	}
  
+	spin_lock(&configfs_dirent_lock);
 	ret = configfs_detach_prep(dentry);
 	if (ret) {
 		configfs_detach_rollback(dentry);
+		spin_unlock(&configfs_dirent_lock);
 		config_item_put(parent_item);
 		return ret;
 	}
+	spin_unlock(&configfs_dirent_lock);
  
 	/* Get a working ref for the duration of this function */
 	item = configfs_get_config_item(dentry);
  
@@ -1476,9 +1479,11 @@
 	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);
+	spin_lock(&configfs_dirent_lock);
 	if (configfs_detach_prep(dentry)) {
 		printk(KERN_ERR "configfs: Tried to unregister non-empty subsystem!\n");
 	}
+	spin_unlock(&configfs_dirent_lock);
 	configfs_detach_group(&group->cg_item);
 	dentry->d_inode->i_flags |= S_DEAD;
 	mutex_unlock(&dentry->d_inode->i_mutex);
...	...	@@ -43,6 +43,10 @@
43	43	* and configfs_dirent_lock locked, in that order.
44	44	* This allows one to safely traverse configfs_dirent trees and symlinks without
45	45	* having to lock inodes.
	46	+ *
	47	+ * Protects setting of CONFIGFS_USET_DROPPING: checking the flag
	48	+ * unlocked is not reliable unless in detach_groups() called from
	49	+ * rmdir()/unregister() and from configfs_attach_group()
46	50	*/
47	51	DEFINE_SPINLOCK(configfs_dirent_lock);
48	52
...	...	@@ -91,6 +95,11 @@
91	95	INIT_LIST_HEAD(&sd->s_children);
92	96	sd->s_element = element;
93	97	spin_lock(&configfs_dirent_lock);
	98	+ if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
	99	+ spin_unlock(&configfs_dirent_lock);
	100	+ kmem_cache_free(configfs_dir_cachep, sd);
	101	+ return ERR_PTR(-ENOENT);
	102	+ }
94	103	list_add(&sd->s_sibling, &parent_sd->s_children);
95	104	spin_unlock(&configfs_dirent_lock);
96	105
...	...	@@ -349,11 +358,11 @@
349	358
350	359	/*
351	360	* Only subdirectories count here. Files (CONFIGFS_NOT_PINNED) are
352		- * attributes and are removed by rmdir(). We recurse, taking i_mutex
353		- * on all children that are candidates for default detach. If the
354		- * result is clean, then configfs_detach_group() will handle dropping
355		- * i_mutex. If there is an error, the caller will clean up the i_mutex
356		- * holders via configfs_detach_rollback().
	361	+ * attributes and are removed by rmdir(). We recurse, setting
	362	+ * CONFIGFS_USET_DROPPING on all children that are candidates for
	363	+ * default detach.
	364	+ * If there is an error, the caller will reset the flags via
	365	+ * configfs_detach_rollback().
357	366	*/
358	367	static int configfs_detach_prep(struct dentry *dentry)
359	368	{
...	...	@@ -370,8 +379,7 @@
370	379	if (sd->s_type & CONFIGFS_NOT_PINNED)
371	380	continue;
372	381	if (sd->s_type & CONFIGFS_USET_DEFAULT) {
373		- mutex_lock(&sd->s_dentry->d_inode->i_mutex);
374		- /* Mark that we've taken i_mutex */
	382	+ /* Mark that we're trying to drop the group */
375	383	sd->s_type \|= CONFIGFS_USET_DROPPING;
376	384
377	385	/*
...	...	@@ -392,7 +400,7 @@
392	400	}
393	401
394	402	/*
395		- * Walk the tree, dropping i_mutex wherever CONFIGFS_USET_DROPPING is
	403	+ * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
396	404	* set.
397	405	*/
398	406	static void configfs_detach_rollback(struct dentry *dentry)
...	...	@@ -403,11 +411,7 @@
403	411	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
404	412	if (sd->s_type & CONFIGFS_USET_DEFAULT) {
405	413	configfs_detach_rollback(sd->s_dentry);
406		-
407		- if (sd->s_type & CONFIGFS_USET_DROPPING) {
408		- sd->s_type &= ~CONFIGFS_USET_DROPPING;
409		- mutex_unlock(&sd->s_dentry->d_inode->i_mutex);
410		- }
	414	+ sd->s_type &= ~CONFIGFS_USET_DROPPING;
411	415	}
412	416	}
413	417	}
414	418
...	...	@@ -486,16 +490,12 @@
486	490
487	491	child = sd->s_dentry;
488	492
	493	+ mutex_lock(&child->d_inode->i_mutex);
	494	+
489	495	configfs_detach_group(sd->s_element);
490	496	child->d_inode->i_flags \|= S_DEAD;
491	497
492		- /*
493		- * From rmdir/unregister, a configfs_detach_prep() pass
494		- * has taken our i_mutex for us. Drop it.
495		- * From mkdir/register cleanup, there is no sem held.
496		- */
497		- if (sd->s_type & CONFIGFS_USET_DROPPING)
498		- mutex_unlock(&child->d_inode->i_mutex);
	498	+ mutex_unlock(&child->d_inode->i_mutex);
499	499
500	500	d_delete(child);
501	501	dput(child);
502	502
503	503
...	...	@@ -1181,12 +1181,15 @@
1181	1181	return -EINVAL;
1182	1182	}
1183	1183
	1184	+ spin_lock(&configfs_dirent_lock);
1184	1185	ret = configfs_detach_prep(dentry);
1185	1186	if (ret) {
1186	1187	configfs_detach_rollback(dentry);
	1188	+ spin_unlock(&configfs_dirent_lock);
1187	1189	config_item_put(parent_item);
1188	1190	return ret;
1189	1191	}
	1192	+ spin_unlock(&configfs_dirent_lock);
1190	1193
1191	1194	/* Get a working ref for the duration of this function */
1192	1195	item = configfs_get_config_item(dentry);
1193	1196
...	...	@@ -1476,9 +1479,11 @@
1476	1479	mutex_lock_nested(&configfs_sb->s_root->d_inode->i_mutex,
1477	1480	I_MUTEX_PARENT);
1478	1481	mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD);
	1482	+ spin_lock(&configfs_dirent_lock);
1479	1483	if (configfs_detach_prep(dentry)) {
1480	1484	printk(KERN_ERR "configfs: Tried to unregister non-empty subsystem!\n");
1481	1485	}
	1486	+ spin_unlock(&configfs_dirent_lock);
1482	1487	configfs_detach_group(&group->cg_item);
1483	1488	dentry->d_inode->i_flags \|= S_DEAD;
1484	1489	mutex_unlock(&dentry->d_inode->i_mutex);