linux-kernel - [PATCH] epoll: Do not take the nested ep->mtx on EPOLL_CTL

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20131226220353.254A9FE054@prod-mail-relay02.akamai.com>
Date:	Thu, 26 Dec 2013 22:03:53 +0000 (GMT)
From:	Jason Baron <jbaron@...mai.com>
To:	akpm@...ux-foundation.org, torvalds@...ux-foundation.org
Cc:	paulmck@...ux.vnet.ibm.com, normalperson@...t.net, nzimmer@....com,
	viro@...iv.linux.org.uk, nelhage@...hage.com,
	davidel@...ilserver.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: [PATCH] epoll: Do not take the nested ep->mtx on EPOLL_CTL_DEL

The EPOLL_CTL_DEL path of epoll contains a classic, ab-ba deadlock. That is,
epoll_ctl(a, EPOLL_CTL_DEL, b, x), will deadlock with epoll_ctl(b,
EPOLL_CTL_DEL, a, x). The deadlock was introduced with commmit 67347fe4e6326
(epoll: do not take global 'epmutex' for simple topologies).

The acquistion of the ep->mtx for the destination 'ep' was added such that a
concurrent EPOLL_CTL_ADD operation would see the correct state of the ep.
(Specifically, the check for '!list_empty(&f.file->f_ep_links'.)
However, by simply not acquiring the lock, we do not serialize behind
the ep->mtx from the add path, and thus may perform a full path check when if we
had waited a little longer it may not have been necessary. However, this is a
transient state, and performing the full loop checking in this case is not
harmful.

The important point is that we wouldn't miss doing the full loop checking when
required, since EPOLL_CTL_ADD always locks any 'ep's that its operating upon.
The reason we don't need to do lock ordering in the add path, is that
we are already are holding the global 'epmutex' whenever we do the
double lock. Further, the original posting of this patch, which was tested
for the intended performance gains, did not perform this additional locking.

Signed-off-by: Jason Baron <jbaron@...mai.com>
---
 fs/eventpoll.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 8b5e258..af90312 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1907,10 +1907,6 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
 			}
 		}
 	}
-	if (op == EPOLL_CTL_DEL && is_file_epoll(tf.file)) {
-		tep = tf.file->private_data;
-		mutex_lock_nested(&tep->mtx, 1);
-	}

 	/*
 	 * Try to lookup the file inside our RB tree, Since we grabbed "mtx"
-- 
1.8.2

--
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/