linux-kernel - [PATCH 004 of 8] knfsd: Close a race-opportunity in d_splice

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]

Message-Id:  <1060929030858.24062@suse.de>
Date:	Fri, 29 Sep 2006 13:08:58 +1000
From:	NeilBrown <neilb@...e.de>
To:	Andrew Morton <akpm@...l.org>
Cc:	nfs@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: [PATCH 004 of 8] knfsd: Close a race-opportunity in d_splice_alias

There is a possible race in d_splice_alias.
Though __d_find_alias(inode, 1) will only return a dentry
with DCACHE_DISCONNECTED set, it is possible for it to get cleared
before the BUG_ON, and it is is not possible to lock against that.

There are a couple of problems here.
Firstly, the code doesn't match the comment.  The comment describes
a 'disconnected' dentry as being IS_ROOT as well as DCACHE_DISCONNECTED,
however there is not testing of IS_ROOT anythere.

A dentry is marked DCACHE_DISCONNECTED when allocated with
d_alloc_anon, and remains DCACHE_DISCONNECTED while a path is built up
towards the root.  So a dentry can have a valid name and a valid
parent and even grandparent, but will still be DCACHE_DISCONNECTED
until a path to the root is created.  Once the path to the root is
complete, everything in the path gets DCACHE_DISCONNECTED cleared.  So
the fact that DCACHE_DISCONNECTED isn't enough to say that a dentry is
free to be spliced in with a given name.  This can only be allowed if
the dentry does not yet have a name, so the IS_ROOT test is needed too.

However even adding that test to __d_find_alias isn't enough.  As
d_splice_alias drops dcache_lock before calling d_move to perform the
splice, it could race with another thread calling d_splice_alias to
splice the inode in with a different name in a different part of the
tree (in the case where a file has hard links).  So that splicing code
is only really safe for directories (as we know that directories only
have one link).  For directories, the caller of d_splice_alias will be
holding i_mutex on the (unique) parent so there is no room for a race.

A consequence of this is that a non-directory will never benefit from
being spliced into a pre-exisiting dentry, but that isn't a problem.
It is perfectly OK for a non-directory to have multiple dentries, some
anonymous, some not.  And the comment for d_splice_alias says that it
only happens for directories anyway.

Signed-off-by: Neil Brown <neilb@...e.de>

### Diffstat output
 ./fs/dcache.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff .prev/fs/dcache.c ./fs/dcache.c
--- .prev/fs/dcache.c	2006-09-29 12:57:26.000000000 +1000
+++ ./fs/dcache.c	2006-09-29 12:57:34.000000000 +1000
@@ -291,9 +291,9 @@ struct dentry * dget_locked(struct dentr
  * it can be unhashed only if it has no children, or if it is the root
  * of a filesystem.
  *
- * If the inode has a DCACHE_DISCONNECTED alias, then prefer
+ * If the inode has an IS_ROOT, DCACHE_DISCONNECTED alias, then prefer
  * any other hashed alias over that one unless @want_discon is set,
- * in which case only return a DCACHE_DISCONNECTED alias.
+ * in which case only return an IS_ROOT, DCACHE_DISCONNECTED alias.
  */

 static struct dentry * __d_find_alias(struct inode *inode, int want_discon)
@@ -309,7 +309,8 @@ static struct dentry * __d_find_alias(st
 		prefetch(next);
 		alias = list_entry(tmp, struct dentry, d_alias);
  		if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
-			if (alias->d_flags & DCACHE_DISCONNECTED)
+			if (IS_ROOT(alias) &&
+			    (alias->d_flags & DCACHE_DISCONNECTED))
 				discon_alias = alias;
 			else if (!want_discon) {
 				__dget_locked(alias);
@@ -1134,7 +1135,7 @@ struct dentry *d_splice_alias(struct ino
 {
 	struct dentry *new = NULL;

-	if (inode) {
+	if (inode && S_ISDIR(inode->i_mode)) {
 		spin_lock(&dcache_lock);
 		new = __d_find_alias(inode, 1);
 		if (new) {
-
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/