| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Jul 2013 21:27:42 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Al Viro <viro@...iv.linux.org.uk>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Alexander Z Lam <azl@...gle.com>,
Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
David Sharp <dhsharp@...gle.com>,
Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
Vaibhav Nagarnaik <vnagarnaik@...gle.com>,
"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
linux-kernel@...r.kernel.org
Subject: PATCH? debugfs_remove_recursive() must not rely on
list_empty(d_subdirs)
Hello.
debugfs_remove_recursive() looks wrong to me. The patch below is
probably wrong too and it was only slightly tested, but could you
please confirm my understanding?
First of all, the usage of simple_release_fs() doesn't look correct
but please ignore, the patch doesn't try to fix this.
The problem is, debugfs_remove_recursive() wrongly (I think) assumes
that a) !list_empty(d_subdirs) means that this dir should be removed,
and b) if d_subdirs does not becomes empty after __debugfs_remove()
debugfs_remove_recursive() gives up and doesn't even try to remove
other entries.
I think this is wrong, and at least we need the additional
debugfs_positive() check before list_empty() or just simple_empty()
instead.
Test-case. Suppose we have
dir1/
dir2/
file2
file1
somewhere in debugfs.
Suppose that someone opens dir1/dir2/file2 and keeps it opened.
Now. debugfs_remove_recursive(dir1/dir2) succeeds, and dir1/di2 goes
away.
But debugfs_remove_recursive(dir1) silently fails and doesn't remove
this directory. Because it tries to delete (the already deleted)
dir1/dir2/file2 again and then fails due to "Avoid infinite loop"
logic.
The patch seems to fix the problem. Note also that the new code
tries to remove as much as possible, iow it does not give up if
__debugfs_remove() fails for any reason. However I am not sure
we want this, perhaps it should simply abort and return the err.
To simplify the review, this is how it looks with the patch applied:
void debugfs_remove_recursive(struct dentry *dentry)
{
struct dentry *child, *next, *parent;
if (IS_ERR_OR_NULL(dentry))
return;
parent = dentry->d_parent;
if (!parent || !parent->d_inode)
return;
parent = dentry;
down:
mutex_lock(&parent->d_inode->i_mutex);
child = list_first_entry(&parent->d_subdirs, struct dentry, d_u.d_child);
while (&child->d_u.d_child != &parent->d_subdirs) {
next = list_next_entry(child, d_u.d_child);
if (!debugfs_positive(child))
goto next;
/* XXX: simple_empty(child) instead ? */
if (!list_empty(&child->d_subdirs)) {
mutex_unlock(&parent->d_inode->i_mutex);
parent = child;
goto down;
}
up:
__debugfs_remove(child, parent);
next:
child = next;
}
mutex_unlock(&parent->d_inode->i_mutex);
if (parent != dentry) {
child = parent;
next = list_next_entry(child, d_u.d_child);
parent = parent->d_parent;
mutex_lock(&parent->d_inode->i_mutex);
goto up;
}
parent = dentry->d_parent;
mutex_lock(&parent->d_inode->i_mutex);
__debugfs_remove(dentry, parent);
mutex_unlock(&parent->d_inode->i_mutex);
simple_release_fs(&debugfs_mount, &debugfs_mount_count);
}
Oleg.
---
debugfs/inode.c | 69 +++++++++++++++++++++-----------------------------------
1 file changed, 26 insertions(+), 43 deletions(-)
--- x/fs/debugfs/inode.c 2013-07-25 19:08:08.000000000 +0200
+++ x/fs/debugfs/inode.c 2013-07-25 21:18:26.000000000 +0200
@@ -532,6 +532,9 @@ void debugfs_remove(struct dentry *dentr
}
EXPORT_SYMBOL_GPL(debugfs_remove);
+#define list_next_entry(pos, member) \
+ list_entry(pos->member.next, typeof(*pos), member)
+
/**
* debugfs_remove_recursive - recursively removes a directory
* @dentry: a pointer to a the dentry of the directory to be removed.
@@ -546,8 +549,7 @@ EXPORT_SYMBOL_GPL(debugfs_remove);
*/
void debugfs_remove_recursive(struct dentry *dentry)
{
- struct dentry *child;
- struct dentry *parent;
+ struct dentry *child, *next, *parent;
if (IS_ERR_OR_NULL(dentry))
return;
@@ -557,54 +559,35 @@ void debugfs_remove_recursive(struct den
return;
parent = dentry;
+ down:
mutex_lock(&parent->d_inode->i_mutex);
+ child = list_first_entry(&parent->d_subdirs, struct dentry, d_u.d_child);
- while (1) {
- /*
- * When all dentries under "parent" has been removed,
- * walk up the tree until we reach our starting point.
- */
- if (list_empty(&parent->d_subdirs)) {
- mutex_unlock(&parent->d_inode->i_mutex);
- if (parent == dentry)
- break;
- parent = parent->d_parent;
- mutex_lock(&parent->d_inode->i_mutex);
- }
- child = list_entry(parent->d_subdirs.next, struct dentry,
- d_u.d_child);
- next_sibling:
-
- /*
- * If "child" isn't empty, walk down the tree and
- * remove all its descendants first.
- */
+ while (&child->d_u.d_child != &parent->d_subdirs) {
+ next = list_next_entry(child, d_u.d_child);
+
+ if (!debugfs_positive(child))
+ goto next;
+
+ /* XXX: simple_empty(child) instead ? */
if (!list_empty(&child->d_subdirs)) {
mutex_unlock(&parent->d_inode->i_mutex);
parent = child;
- mutex_lock(&parent->d_inode->i_mutex);
- continue;
+ goto down;
}
+ up:
__debugfs_remove(child, parent);
- if (parent->d_subdirs.next == &child->d_u.d_child) {
- /*
- * Try the next sibling.
- */
- if (child->d_u.d_child.next != &parent->d_subdirs) {
- child = list_entry(child->d_u.d_child.next,
- struct dentry,
- d_u.d_child);
- goto next_sibling;
- }
-
- /*
- * Avoid infinite loop if we fail to remove
- * one dentry.
- */
- mutex_unlock(&parent->d_inode->i_mutex);
- break;
- }
- simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+ next:
+ child = next;
+ }
+
+ mutex_unlock(&parent->d_inode->i_mutex);
+ if (parent != dentry) {
+ child = parent;
+ next = list_next_entry(child, d_u.d_child);
+ parent = parent->d_parent;
+ mutex_lock(&parent->d_inode->i_mutex);
+ goto up;
}
parent = dentry->d_parent;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists