The patch titled epoll: fix for epoll_wait() sometimes returning events on closed fds has been added to the -mm tree. Its filename is epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds.patch Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's *** Remember to use Documentation/SubmitChecklist when testing your code *** See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find out what to do about this The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/ ------------------------------------------------------ Subject: epoll: fix for epoll_wait() sometimes returning events on closed fds From: Tony Battersby <tonyb@xxxxxxxxxxxxxxx> The epoll manpage: Q: Will closing a file descriptor cause it to be removed from all epoll sets automatically? A: Yes sys_close() calls filp_close(), which calls fput(). If no one else holds a reference to the file, then fput() calls __fput(), and __fput() calls eventpoll_release(), which prevents epoll_wait() from returning events on the fd to userspace. In the rare case that sys_close() doesn't call __fput() because someone else has a reference to the file, a subsequent epoll_wait() may still unexpectedly return events on the fd after it has been closed. This can end up confusing or crashing a userspace program that doesn't do epoll_ctl(EPOLL_CTL_DEL) before closing the fd. I have reports of this actually happening under heavy load with a program using epoll with network sockets on 2.6.27. This patch fixes the problem by calling eventpoll_release_file() from filp_close() instead of from __fput(). The locking in eventpoll_release() and eventpoll_release_file() needs to be changed because previously it relied on the fact that no one else could have a reference to the file when called from __fput(), and this is no longer true. The new locking is admittedly ugly, but I believe it works. ep_insert() now also needs to check if the file has been closed to avoid races in multi-threaded programs where one thread is doing epoll_ctl(EPOLL_CTL_ADD) and another thread is closing the fd. This is done by checking that fget_light still returns the same struct file * as before. Note that the list_del_init(&epi->fllink) previously done in eventpoll_release_file() was unnecessary because it is also done by ep_remove(). Userspace programs that might run on kernels with this bug can work around the problem by doing epoll_ctl(EPOLL_CTL_DEL) before close(). Signed-off-by: Tony Battersby <tonyb@xxxxxxxxxxxxxxx> Cc: <stable@xxxxxxxxxx> Cc: Michael Kerrisk <mtk.manpages@xxxxxxxxxxxxxx> Cc: Davide Libenzi <davidel@xxxxxxxxxxxxxxx> Cc: Jonathan Corbet <corbet@xxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- fs/eventpoll.c | 66 ++++++++++++++++++++++++++++-------- fs/file_table.c | 5 -- fs/open.c | 2 + include/linux/eventpoll.h | 16 ++++---- 4 files changed, 64 insertions(+), 25 deletions(-) diff -puN fs/eventpoll.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds fs/eventpoll.c --- a/fs/eventpoll.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds +++ a/fs/eventpoll.c @@ -538,27 +538,40 @@ void eventpoll_release_file(struct file struct epitem *epi; /* - * We don't want to get "file->f_ep_lock" because it is not - * necessary. It is not necessary because we're in the "struct file" - * cleanup path, and this means that noone is using this file anymore. - * So, for example, epoll_ctl() cannot hit here sicne 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(). + * epmutex makes it safe to use *ep by preventing ep_free() from + * running. */ mutex_lock(&epmutex); + spin_lock(&file->f_ep_lock); while (!list_empty(lsthead)) { - epi = list_first_entry(lsthead, struct epitem, fllink); + ep = list_first_entry(lsthead, struct epitem, fllink)->ep; + spin_unlock(&file->f_ep_lock); - ep = epi->ep; - list_del_init(&epi->fllink); + /* + * ep->mtx protects against epoll_ctl() and is required for + * calling ep_remove(). + */ mutex_lock(&ep->mtx); - ep_remove(ep, epi); + + spin_lock(&file->f_ep_lock); + epi = list_first_entry(lsthead, struct epitem, fllink); + spin_unlock(&file->f_ep_lock); + + /* + * epoll_ctl(EPOLL_CTL_DEL) may have modified the list before + * we locked ep->mtx, so it is necessary to check that the + * first entry still exists and belongs to the ep whose mtx we + * are holding. + */ + if (epi && epi->ep == ep) + ep_remove(ep, epi); + mutex_unlock(&ep->mtx); + + spin_lock(&file->f_ep_lock); } + spin_unlock(&file->f_ep_lock); mutex_unlock(&epmutex); } @@ -736,6 +749,22 @@ static void ep_rbtree_insert(struct even } /* + * Returns true if the fd has been closed or false if not. + */ +static bool ep_is_file_closed(struct file *file, int fd) +{ + struct file *file2; + int needs_fput; + bool closed; + + file2 = fget_light(fd, &needs_fput); + closed = (file2 != file); + if (file2) + fput_light(file2, needs_fput); + return closed; +} + +/* * Must be called with "mtx" held. */ static int ep_insert(struct eventpoll *ep, struct epoll_event *event, @@ -786,6 +815,17 @@ static int ep_insert(struct eventpoll *e /* Add the current item to the list of active epoll hook for this file */ spin_lock(&tfile->f_ep_lock); + /* + * Don't add the fd to the epoll set if it was closed in between the + * initial fget() and the time we get here. This check must be done + * while holding f_ep_lock to prevent races with + * eventpoll_release_file(). + */ + if (ep_is_file_closed(tfile, fd)) { + spin_unlock(&tfile->f_ep_lock); + error = -EBADF; + goto error_unregister; + } list_add_tail(&epi->fllink, &tfile->f_ep_links); spin_unlock(&tfile->f_ep_lock); diff -puN fs/file_table.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds fs/file_table.c --- a/fs/file_table.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds +++ a/fs/file_table.c @@ -266,11 +266,6 @@ void __fput(struct file *file) might_sleep(); fsnotify_close(file); - /* - * The function eventpoll_release() should be the first called - * in the file cleanup chain. - */ - eventpoll_release(file); locks_remove_flock(file); if (unlikely(file->f_flags & FASYNC)) { diff -puN fs/open.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds fs/open.c --- a/fs/open.c~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds +++ a/fs/open.c @@ -29,6 +29,7 @@ #include <linux/rcupdate.h> #include <linux/audit.h> #include <linux/falloc.h> +#include <linux/eventpoll.h> int vfs_statfs(struct dentry *dentry, struct kstatfs *buf) { @@ -1104,6 +1105,7 @@ int filp_close(struct file *filp, fl_own dnotify_flush(filp, id); locks_remove_posix(filp, id); + eventpoll_release(filp); fput(filp); return retval; } diff -puN include/linux/eventpoll.h~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds include/linux/eventpoll.h --- a/include/linux/eventpoll.h~epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds +++ a/include/linux/eventpoll.h @@ -69,23 +69,25 @@ static inline void eventpoll_init_file(s void eventpoll_release_file(struct file *file); /* - * This is called from inside fs/file_table.c:__fput() to unlink files + * This is called from inside fs/open.c:filp_close() to unlink files * from the eventpoll interface. We need to have this facility to cleanup * correctly files that are closed without being removed from the eventpoll * interface. */ static inline void eventpoll_release(struct file *file) { + bool empty; /* - * Fast check to avoid the get/release of the semaphore. Since - * we're doing this outside the semaphore lock, it might return + * Fast check to avoid the get/release of the mutex. Since + * we're doing this outside the mutex lock, it might return * false negatives, but we don't care. It'll help in 99.99% of cases - * to avoid the semaphore lock. False positives simply cannot happen - * because the file in on the way to be removed and nobody ( but - * eventpoll ) has still a reference to this file. + * to avoid the mutex lock. */ - if (likely(list_empty(&file->f_ep_links))) + spin_lock(&file->f_ep_lock); + empty = list_empty(&file->f_ep_links); + spin_unlock(&file->f_ep_lock); + if (likely(empty)) return; /* _ Patches currently in -mm which might be from tonyb@xxxxxxxxxxxxxxx are linux-next.patch vfs-improve-comment-describing-fget_light.patch epoll-fix-for-epoll_wait-sometimes-returning-events-on-closed-fds.patch make-shm_get_stat-more-robust.patch -- To unsubscribe from this list: send the line "unsubscribe mm-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html