[Devel] [PATCH vz9 v3 2/3] Revert "epoll: use refcount to reduce ep_mutex contention"
Eva Kurchatova
eva.kurchatova at virtuozzo.com
Sun Jul 12 22:02:04 MSK 2026
This reverts upstream commit 58c9b016e12855286370dfb704c08498edbc857a,
which was imported by commit b44bae1bd6e953127cc01e86aaa3bb2b09a0dc2d
Eliminates the racy refcount-based disposal and restores the previous
epmutex serialization between ep_free and eventpoll_release_file.
It also removes the struct fields, which the next commit adds back.
Signed-off-by: Eva Kurchatova <eva.kurchatova at virtuozzo.com>
https://virtuozzo.atlassian.net/browse/VSTOR-137490
Feature: fix epoll cve
---
fs/eventpoll.c | 195 ++++++++++++++++++-------------------------------
1 file changed, 72 insertions(+), 123 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 5675fcbd0901..7e6a5b68c8ba 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -57,7 +57,13 @@
* we need a lock that will allow us to sleep. This lock is a
* mutex (ep->mtx). It is acquired during the event transfer loop,
* during epoll_ctl(EPOLL_CTL_DEL) and during eventpoll_release_file().
- * The epmutex is acquired when inserting an epoll fd onto another epoll
+ * Then we also need a global mutex to serialize eventpoll_release_file()
+ * and ep_free().
+ * This mutex is acquired by ep_free() during the epoll file
+ * cleanup path and it is also acquired by eventpoll_release_file()
+ * if a file has been pushed inside an epoll set and it is then
+ * close()d without a previous call to epoll_ctl(EPOLL_CTL_DEL).
+ * It is also acquired when inserting an epoll fd onto another epoll
* fd. We do this so that we walk the epoll tree and ensure that this
* insertion does not create a cycle of epoll file descriptors, which
* could lead to deadlock. We need a global mutex to prevent two
@@ -147,13 +153,6 @@ struct epitem {
/* The file descriptor information this item refers to */
struct epoll_filefd ffd;
- /*
- * Protected by file->f_lock, true for to-be-released epitem already
- * removed from the "struct file" items list; together with
- * eventpoll->refcount orchestrates "struct eventpoll" disposal
- */
- bool dying;
-
/* List containing poll wait queues */
struct eppoll_entry *pwqlist;
@@ -219,12 +218,6 @@ struct eventpoll {
struct hlist_head refs;
u8 loop_check_depth;
- /*
- * usage count, used together with epitem->dying to
- * orchestrate the disposal of this struct
- */
- refcount_t refcount;
-
#ifdef CONFIG_NET_RX_BUSY_POLL
/* used to track busy poll napi_id */
unsigned int napi_id;
@@ -248,7 +241,9 @@ struct ep_pqueue {
/* Maximum number of epoll watched descriptors, per user */
static long max_user_watches __read_mostly;
-/* Used for cycles detection */
+/*
+ * This mutex is used to serialize ep_free() and eventpoll_release_file().
+ */
static DEFINE_MUTEX(epmutex);
static u64 loop_check_gen = 0;
@@ -563,7 +558,8 @@ static void ep_remove_wait_queue(struct eppoll_entry *pwq)
/*
* This function unregisters poll callbacks from the associated file
- * descriptor. Must be called with "mtx" held.
+ * descriptor. Must be called with "mtx" held (or "epmutex" if called from
+ * ep_free).
*/
static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
{
@@ -686,40 +682,11 @@ static void epi_rcu_free(struct rcu_head *head)
kmem_cache_free(epi_cache, epi);
}
-static void ep_get(struct eventpoll *ep)
-{
- refcount_inc(&ep->refcount);
-}
-
-/*
- * Returns true if the event poll can be disposed
- */
-static bool ep_refcount_dec_and_test(struct eventpoll *ep)
-{
- if (!refcount_dec_and_test(&ep->refcount))
- return false;
-
- WARN_ON_ONCE(!RB_EMPTY_ROOT(&ep->rbr.rb_root));
- return true;
-}
-
-static void ep_free(struct eventpoll *ep)
-{
- mutex_destroy(&ep->mtx);
- free_uid(ep->user);
- wakeup_source_unregister(ep->ws);
- kfree(ep);
-}
-
/*
* Removes a "struct epitem" from the eventpoll RB tree and deallocates
* all the associated resources. Must be called with "mtx" held.
- * If the dying flag is set, do the removal only if force is true.
- * This prevents ep_clear_and_put() from dropping all the ep references
- * while running concurrently with eventpoll_release_file().
- * Returns true if the eventpoll can be disposed.
*/
-static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)
+static int ep_remove(struct eventpoll *ep, struct epitem *epi)
{
struct file *file = epi->ffd.file;
struct epitems_head *to_free;
@@ -734,11 +701,6 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)
/* Remove the current item from the list of epoll hooks */
spin_lock(&file->f_lock);
- if (epi->dying && !force) {
- spin_unlock(&file->f_lock);
- return false;
- }
-
to_free = NULL;
head = file->f_ep;
if (head->first == &epi->fllink && !epi->fllink.next) {
@@ -772,28 +734,28 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)
call_rcu(&epi->rcu, epi_rcu_free);
percpu_counter_dec(&ep->user->epoll_watches);
- return ep_refcount_dec_and_test(ep);
-}
-/*
- * ep_remove variant for callers owing an additional reference to the ep
- */
-static void ep_remove_safe(struct eventpoll *ep, struct epitem *epi)
-{
- WARN_ON_ONCE(__ep_remove(ep, epi, false));
+ return 0;
}
-static void ep_clear_and_put(struct eventpoll *ep)
+static void ep_free(struct eventpoll *ep)
{
- struct rb_node *rbp, *next;
+ struct rb_node *rbp;
struct epitem *epi;
- bool dispose;
/* We need to release all tasks waiting for these file */
if (waitqueue_active(&ep->poll_wait))
ep_poll_safewake(ep, NULL, 0);
- mutex_lock(&ep->mtx);
+ /*
+ * We need to lock this because we could be hit by
+ * eventpoll_release_file() while we're freeing the "struct eventpoll".
+ * We do not need to hold "ep->mtx" here because the epoll file
+ * is on the way to be removed and no one has references to it
+ * anymore. The only hit might come from eventpoll_release_file() but
+ * holding "epmutex" is sufficient here.
+ */
+ mutex_lock(&epmutex);
/*
* Walks through the whole tree by unregistering poll callbacks.
@@ -806,25 +768,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
}
/*
- * Walks through the whole tree and try to free each "struct epitem".
- * Note that ep_remove_safe() will not remove the epitem in case of a
- * racing eventpoll_release_file(); the latter will do the removal.
- * At this point we are sure no poll callbacks will be lingering around.
- * Since we still own a reference to the eventpoll struct, the loop can't
- * dispose it.
+ * Walks through the whole tree by freeing each "struct epitem". At this
+ * point we are sure no poll callbacks will be lingering around, and also by
+ * holding "epmutex" we can be sure that no file cleanup code will hit
+ * us during this operation. So we can avoid the lock on "ep->lock".
+ * We do not need to lock ep->mtx, either, we only do it to prevent
+ * a lockdep warning.
*/
- for (rbp = rb_first_cached(&ep->rbr); rbp; rbp = next) {
- next = rb_next(rbp);
+ mutex_lock(&ep->mtx);
+ while ((rbp = rb_first_cached(&ep->rbr)) != NULL) {
epi = rb_entry(rbp, struct epitem, rbn);
- ep_remove_safe(ep, epi);
+ ep_remove(ep, epi);
cond_resched();
}
-
- dispose = ep_refcount_dec_and_test(ep);
mutex_unlock(&ep->mtx);
- if (dispose)
- ep_free(ep);
+ mutex_unlock(&epmutex);
+ mutex_destroy(&ep->mtx);
+ free_uid(ep->user);
+ wakeup_source_unregister(ep->ws);
+ kfree(ep);
}
static int ep_eventpoll_release(struct inode *inode, struct file *file)
@@ -832,7 +795,7 @@ static int ep_eventpoll_release(struct inode *inode, struct file *file)
struct eventpoll *ep = file->private_data;
if (ep)
- ep_clear_and_put(ep);
+ ep_free(ep);
return 0;
}
@@ -980,34 +943,33 @@ void eventpoll_release_file(struct file *file)
{
struct eventpoll *ep;
struct epitem *epi;
- bool dispose;
+ struct hlist_node *next;
/*
- * Use the 'dying' flag to prevent a concurrent ep_clear_and_put() from
- * touching the epitems list before eventpoll_release_file() can access
- * the ep->mtx.
+ * We don't want to get "file->f_lock" because it is not
+ * necessary. It is not necessary because we're in the "struct file"
+ * cleanup path, and this means that no one is using this file anymore.
+ * So, for example, epoll_ctl() cannot hit here since if we reach this
+ * point, the file counter already went to zero and fget() would fail.
+ * The only hit might come from ep_free() but by holding the mutex
+ * will correctly serialize the operation. We do need to acquire
+ * "ep->mtx" after "epmutex" because ep_remove() requires it when called
+ * from anywhere but ep_free().
+ *
+ * Besides, ep_remove() acquires the lock, so we can't hold it here.
*/
-again:
- spin_lock(&file->f_lock);
- if (file->f_ep && file->f_ep->first) {
- epi = hlist_entry(file->f_ep->first, struct epitem, fllink);
- epi->dying = true;
- spin_unlock(&file->f_lock);
-
- /*
- * ep access is safe as we still own a reference to the ep
- * struct
- */
+ mutex_lock(&epmutex);
+ if (unlikely(!file->f_ep)) {
+ mutex_unlock(&epmutex);
+ return;
+ }
+ hlist_for_each_entry_safe(epi, next, file->f_ep, fllink) {
ep = epi->ep;
- mutex_lock(&ep->mtx);
- dispose = __ep_remove(ep, epi, true);
+ mutex_lock_nested(&ep->mtx, 0);
+ ep_remove(ep, epi);
mutex_unlock(&ep->mtx);
-
- if (dispose)
- ep_free(ep);
- goto again;
}
- spin_unlock(&file->f_lock);
+ mutex_unlock(&epmutex);
}
static int ep_alloc(struct eventpoll **pep)
@@ -1030,7 +992,6 @@ static int ep_alloc(struct eventpoll **pep)
ep->rbr = RB_ROOT_CACHED;
ep->ovflist = EP_UNACTIVE_PTR;
ep->user = user;
- refcount_set(&ep->refcount, 1);
*pep = ep;
@@ -1299,10 +1260,10 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
*/
list_del_init(&wait->entry);
/*
- * ->whead != NULL protects us from the race with
- * ep_clear_and_put() or ep_remove(), ep_remove_wait_queue()
- * takes whead->lock held by the caller. Once we nullify it,
- * nothing protects ep/epi or even wait.
+ * ->whead != NULL protects us from the race with ep_free()
+ * or ep_remove(), ep_remove_wait_queue() takes whead->lock
+ * held by the caller. Once we nullify it, nothing protects
+ * ep/epi or even wait.
*/
smp_store_release(&ep_pwq_from_wait(wait)->whead, NULL);
}
@@ -1572,22 +1533,16 @@ static int ep_insert(struct eventpoll *ep, const struct epoll_event *event,
if (tep)
mutex_unlock(&tep->mtx);
- /*
- * ep_remove_safe() calls in the later error paths can't lead to
- * ep_free() as the ep file itself still holds an ep reference.
- */
- ep_get(ep);
-
/* now check if we've created too many backpaths */
if (unlikely(full_check && reverse_path_check())) {
- ep_remove_safe(ep, epi);
+ ep_remove(ep, epi);
return -EINVAL;
}
if (epi->event.events & EPOLLWAKEUP) {
error = ep_create_wakeup_source(epi);
if (error) {
- ep_remove_safe(ep, epi);
+ ep_remove(ep, epi);
return error;
}
}
@@ -1611,7 +1566,7 @@ static int ep_insert(struct eventpoll *ep, const struct epoll_event *event,
* high memory pressure.
*/
if (unlikely(!epq.epi)) {
- ep_remove_safe(ep, epi);
+ ep_remove(ep, epi);
return -ENOMEM;
}
@@ -2117,7 +2072,7 @@ static int do_epoll_create(int flags)
out_free_fd:
put_unused_fd(fd);
out_free_ep:
- ep_clear_and_put(ep);
+ ep_free(ep);
return error;
}
@@ -2259,16 +2214,10 @@ int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds,
error = -EEXIST;
break;
case EPOLL_CTL_DEL:
- if (epi) {
- /*
- * The eventpoll itself is still alive: the refcount
- * can't go to zero here.
- */
- ep_remove_safe(ep, epi);
- error = 0;
- } else {
+ if (epi)
+ error = ep_remove(ep, epi);
+ else
error = -ENOENT;
- }
break;
case EPOLL_CTL_MOD:
if (epi) {
--
2.55.0
More information about the Devel
mailing list