linux-kernel - Re: 2.6.27-rc7-sha1: EIP at proc_sys

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 28 Sep 2008 13:46:34 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...IV.linux.org.uk>
cc:	Alexey Dobriyan <adobriyan@...il.com>, ebiederm@...ssion.com,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: 2.6.27-rc7-sha1: EIP at proc_sys_compare+0x36/0x50



On Sun, 28 Sep 2008, Al Viro wrote:
>
> > Al, did I miss something?
> 
> The real underlying bug, whatever it is.  If this sucker ever becomes
> negative, we have a big problem.  Where _could_ that happen?  Remember,
> we do not allow ->rmdir() and ->unlink() to succeed there.

What about pure memory pressure? We're holding only the RCU read-side lock 
when looking up dentries, and if there is any memory pressure, the 
dentries may be unhashed and the inodes removed in parallel. Yes, yes, we 
end up not actually _releasing_ the dentry, since it's all RCU, but it 
will set D_UNHASHED and be scheduled for releasing later under RCU.

And d_compare() is called before we have done any validation that the name 
is still active, including checking whether it even got released already! 

I dunno. Do we want to move the D_UNHASHED check up earlier? Or am I still 
missing something?

		Linus

----
 fs/dcache.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 80e9395..e7a1a99 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1395,6 +1395,10 @@ struct dentry * __d_lookup(struct dentry * parent, struct qstr * name)
 		if (dentry->d_parent != parent)
 			goto next;
 
+		/* non-existing due to RCU? */
+		if (d_unhashed(dentry))
+			goto next;
+
 		/*
 		 * It is safe to compare names since d_move() cannot
 		 * change the qstr (protected by d_lock).
@@ -1410,10 +1414,8 @@ struct dentry * __d_lookup(struct dentry * parent, struct qstr * name)
 				goto next;
 		}
 
-		if (!d_unhashed(dentry)) {
-			atomic_inc(&dentry->d_count);
-			found = dentry;
-		}
+		atomic_inc(&dentry->d_count);
+		found = dentry;
 		spin_unlock(&dentry->d_lock);
 		break;
 next:
--
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/