lkml.org 
[lkml]   [2013]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectPATCH? 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;


    \
     
     \ /
      Last update: 2013-07-25 22:21    [W:4.326 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site