| 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
| ||
|
Message-ID: <1430097632.4063.156.camel@decadent.org.uk>
Date: Mon, 27 Apr 2015 02:20:32 +0100
From: Ben Hutchings <ben@...adent.org.uk>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, stable@...r.kernel.org,
Al Viro <viro@...iv.linux.org.uk>,
hujianyang <hujianyang@...wei.com>
Subject: Re: [PATCH 3.10 27/31] deal with deadlock in d_walk()
On Sun, 2015-04-26 at 15:49 +0200, Greg Kroah-Hartman wrote:
> 3.10-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Al Viro <viro@...iv.linux.org.uk>
>
> commit ca5358ef75fc69fee5322a38a340f5739d997c10 upstream.
>
> ... by not hitting rename_retry for reasons other than rename having
> happened. In other words, do _not_ restart when finding that
> between unlocking the child and locking the parent the former got
> into __dentry_kill(). Skip the killed siblings instead...
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> Cc: Ben Hutchings <ben@...adent.org.uk>
> [hujianyang: Backported to 3.10 refer to the work of Ben Hutchings in 3.2:
> - As we only have try_to_ascend() and not d_walk(), apply this
> change to all callers of try_to_ascend()
> - Adjust context to make __dentry_kill() apply to d_kill()]
> Signed-off-by: hujianyang <hujianyang@...wei.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
This is broken; you need to fold in commit 20defcec264c from 3.2.y
("dcache: Fix locking bugs in backported "deal with deadlock in
d_walk()"").
Ben.
> ---
> fs/dcache.c | 102 ++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 62 insertions(+), 40 deletions(-)
>
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -364,9 +364,9 @@ static struct dentry *d_kill(struct dent
> __releases(parent->d_lock)
> __releases(dentry->d_inode->i_lock)
> {
> - list_del(&dentry->d_child);
> + __list_del_entry(&dentry->d_child);
> /*
> - * Inform try_to_ascend() that we are no longer attached to the
> + * Inform ascending readers that we are no longer attached to the
> * dentry tree
> */
> dentry->d_flags |= DCACHE_DENTRY_KILLED;
> @@ -988,35 +988,6 @@ void shrink_dcache_for_umount(struct sup
> }
>
> /*
> - * This tries to ascend one level of parenthood, but
> - * we can race with renaming, so we need to re-check
> - * the parenthood after dropping the lock and check
> - * that the sequence number still matches.
> - */
> -static struct dentry *try_to_ascend(struct dentry *old, int locked, unsigned seq)
> -{
> - struct dentry *new = old->d_parent;
> -
> - rcu_read_lock();
> - spin_unlock(&old->d_lock);
> - spin_lock(&new->d_lock);
> -
> - /*
> - * might go back up the wrong parent if we have had a rename
> - * or deletion
> - */
> - if (new != old->d_parent ||
> - (old->d_flags & DCACHE_DENTRY_KILLED) ||
> - (!locked && read_seqretry(&rename_lock, seq))) {
> - spin_unlock(&new->d_lock);
> - new = NULL;
> - }
> - rcu_read_unlock();
> - return new;
> -}
> -
> -
> -/*
> * Search for at least 1 mount point in the dentry's subdirs.
> * We descend to the next level whenever the d_subdirs
> * list is non-empty and continue searching.
> @@ -1070,17 +1041,32 @@ resume:
> /*
> * All done at this level ... ascend and resume the search.
> */
> + rcu_read_lock();
> +ascend:
> if (this_parent != parent) {
> struct dentry *child = this_parent;
> - this_parent = try_to_ascend(this_parent, locked, seq);
> - if (!this_parent)
> + this_parent = child->d_parent;
> +
> + spin_unlock(&child->d_lock);
> + spin_lock(&this_parent->d_lock);
> +
> + /* might go back up the wrong parent if we have had a rename. */
> + if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> next = child->d_child.next;
> + while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> + if (next == &this_parent->d_subdirs)
> + goto ascend;
> + child = list_entry(next, struct dentry, d_child);
> + next = next->next;
> + }
> + rcu_read_unlock();
> goto resume;
> }
> - spin_unlock(&this_parent->d_lock);
> if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (locked)
> write_sequnlock(&rename_lock);
> return 0; /* No mount points found in tree */
> @@ -1092,6 +1078,8 @@ positive:
> return 1;
>
> rename_retry:
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (locked)
> goto again;
> locked = 1;
> @@ -1177,23 +1165,40 @@ resume:
> /*
> * All done at this level ... ascend and resume the search.
> */
> + rcu_read_lock();
> +ascend:
> if (this_parent != parent) {
> struct dentry *child = this_parent;
> - this_parent = try_to_ascend(this_parent, locked, seq);
> - if (!this_parent)
> + this_parent = child->d_parent;
> +
> + spin_unlock(&child->d_lock);
> + spin_lock(&this_parent->d_lock);
> +
> + /* might go back up the wrong parent if we have had a rename. */
> + if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> next = child->d_child.next;
> + while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> + if (next == &this_parent->d_subdirs)
> + goto ascend;
> + child = list_entry(next, struct dentry, d_child);
> + next = next->next;
> + }
> + rcu_read_unlock();
> goto resume;
> }
> out:
> - spin_unlock(&this_parent->d_lock);
> if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (locked)
> write_sequnlock(&rename_lock);
> return found;
>
> rename_retry:
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (found)
> return found;
> if (locked)
> @@ -2954,26 +2959,43 @@ resume:
> }
> spin_unlock(&dentry->d_lock);
> }
> + rcu_read_lock();
> +ascend:
> if (this_parent != root) {
> struct dentry *child = this_parent;
> if (!(this_parent->d_flags & DCACHE_GENOCIDE)) {
> this_parent->d_flags |= DCACHE_GENOCIDE;
> this_parent->d_count--;
> }
> - this_parent = try_to_ascend(this_parent, locked, seq);
> - if (!this_parent)
> + this_parent = child->d_parent;
> +
> + spin_unlock(&child->d_lock);
> + spin_lock(&this_parent->d_lock);
> +
> + /* might go back up the wrong parent if we have had a rename. */
> + if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> next = child->d_child.next;
> + while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> + if (next == &this_parent->d_subdirs)
> + goto ascend;
> + child = list_entry(next, struct dentry, d_child);
> + next = next->next;
> + }
> + rcu_read_unlock();
> goto resume;
> }
> - spin_unlock(&this_parent->d_lock);
> if (!locked && read_seqretry(&rename_lock, seq))
> goto rename_retry;
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (locked)
> write_sequnlock(&rename_lock);
> return;
>
> rename_retry:
> + spin_unlock(&this_parent->d_lock);
> + rcu_read_unlock();
> if (locked)
> goto again;
> locked = 1;
>
>
--
Ben Hutchings
I'm not a reverse psychological virus. Please don't copy me into your sig.
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)
Powered by blists - more mailing lists