From cf04b8d6ac710c83dc9a433aea3e5d3c451095a1 Mon Sep 17 00:00:00 2001 From: Zebediah Figura Date: Wed, 2 Oct 2019 20:37:48 -0500 Subject: [PATCH] eventfd_synchronization: Cherry-pick some fixes. --- ...Only-signal-the-APC-fd-for-user-APCs.patch | 26 +++ .../0086-ntdll-Check-the-APC-fd-first.patch | 43 ++++ ...c-Lock-accessing-the-shm_addrs-array.patch | 52 +++++ ...-the-per-event-spinlock-for-auto-res.patch | 206 ++++++++++++++++++ patches/patchinstall.sh | 8 + 5 files changed, 335 insertions(+) create mode 100644 patches/eventfd_synchronization/0085-server-Only-signal-the-APC-fd-for-user-APCs.patch create mode 100644 patches/eventfd_synchronization/0086-ntdll-Check-the-APC-fd-first.patch create mode 100644 patches/eventfd_synchronization/0087-ntdll-esync-Lock-accessing-the-shm_addrs-array.patch create mode 100644 patches/eventfd_synchronization/0088-ntdll-Get-rid-of-the-per-event-spinlock-for-auto-res.patch diff --git a/patches/eventfd_synchronization/0085-server-Only-signal-the-APC-fd-for-user-APCs.patch b/patches/eventfd_synchronization/0085-server-Only-signal-the-APC-fd-for-user-APCs.patch new file mode 100644 index 00000000..259618a5 --- /dev/null +++ b/patches/eventfd_synchronization/0085-server-Only-signal-the-APC-fd-for-user-APCs.patch @@ -0,0 +1,26 @@ +From 9e4df70f7282b04849153b3fa2edf15dc24eaf4f Mon Sep 17 00:00:00 2001 +From: Zebediah Figura +Date: Tue, 23 Jul 2019 18:39:06 -0500 +Subject: [PATCH] server: Only signal the APC fd for user APCs. + +Otherwise we might incorrectly return WAIT_IO_COMPLETION to the user when a system APC runs. +--- + server/thread.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/server/thread.c b/server/thread.c +index fc751c2cb..2e77e5ff2 100644 +--- a/server/thread.c ++++ b/server/thread.c +@@ -1057,7 +1057,7 @@ static int queue_apc( struct process *process, struct thread *thread, struct thr + { + wake_thread( thread ); + +- if (do_esync()) ++ if (do_esync() && queue == &thread->user_apc) + esync_wake_fd( thread->esync_apc_fd ); + } + +-- +2.23.0 + diff --git a/patches/eventfd_synchronization/0086-ntdll-Check-the-APC-fd-first.patch b/patches/eventfd_synchronization/0086-ntdll-Check-the-APC-fd-first.patch new file mode 100644 index 00000000..6dfd55c7 --- /dev/null +++ b/patches/eventfd_synchronization/0086-ntdll-Check-the-APC-fd-first.patch @@ -0,0 +1,43 @@ +From 836f1b6b0560bd178efb8d52900b4b136f87ae30 Mon Sep 17 00:00:00 2001 +From: Zebediah Figura +Date: Tue, 23 Jul 2019 17:22:20 -0500 +Subject: [PATCH] ntdll: Check the APC fd first. + +--- + dlls/ntdll/esync.c | 13 ++++++++----- + 1 file changed, 8 insertions(+), 5 deletions(-) + +diff --git a/dlls/ntdll/esync.c b/dlls/ntdll/esync.c +index fc621ccfb..0adb4ad77 100644 +--- a/dlls/ntdll/esync.c ++++ b/dlls/ntdll/esync.c +@@ -1046,6 +1046,14 @@ static NTSTATUS __esync_wait_objects( DWORD count, const HANDLE *handles, + ret = do_poll( fds, pollcount, timeout ? &end : NULL ); + if (ret > 0) + { ++ /* We must check this first! The server may set an event that ++ * we're waiting on, but we need to return STATUS_USER_APC. */ ++ if (alertable) ++ { ++ if (fds[pollcount - 1].revents & POLLIN) ++ goto userapc; ++ } ++ + /* Find out which object triggered the wait. */ + for (i = 0; i < count; i++) + { +@@ -1089,11 +1097,6 @@ static NTSTATUS __esync_wait_objects( DWORD count, const HANDLE *handles, + return count - 1; + } + } +- if (alertable) +- { +- if (fds[i++].revents & POLLIN) +- goto userapc; +- } + + /* If we got here, someone else stole (or reset, etc.) whatever + * we were waiting for. So keep waiting. */ +-- +2.23.0 + diff --git a/patches/eventfd_synchronization/0087-ntdll-esync-Lock-accessing-the-shm_addrs-array.patch b/patches/eventfd_synchronization/0087-ntdll-esync-Lock-accessing-the-shm_addrs-array.patch new file mode 100644 index 00000000..771d8556 --- /dev/null +++ b/patches/eventfd_synchronization/0087-ntdll-esync-Lock-accessing-the-shm_addrs-array.patch @@ -0,0 +1,52 @@ +From c1804983dc8e9509c088c35914212cda1bd5a48a Mon Sep 17 00:00:00 2001 +From: Zebediah Figura +Date: Wed, 7 Aug 2019 17:14:54 -0500 +Subject: [PATCH] ntdll/esync: Lock accessing the shm_addrs array. + +--- + dlls/ntdll/esync.c | 18 +++++++++++++++++- + 1 file changed, 17 insertions(+), 1 deletion(-) + +diff --git a/dlls/ntdll/esync.c b/dlls/ntdll/esync.c +index 0adb4ad77..2f030c141 100644 +--- a/dlls/ntdll/esync.c ++++ b/dlls/ntdll/esync.c +@@ -155,10 +155,22 @@ void esync_init(void) + shm_addrs_size = 128; + } + ++static RTL_CRITICAL_SECTION shm_addrs_section; ++static RTL_CRITICAL_SECTION_DEBUG shm_addrs_debug = ++{ ++ 0, 0, &shm_addrs_section, ++ { &shm_addrs_debug.ProcessLocksList, &shm_addrs_debug.ProcessLocksList }, ++ 0, 0, { (DWORD_PTR)(__FILE__ ": shm_addrs_section") } ++}; ++static RTL_CRITICAL_SECTION shm_addrs_section = { &shm_addrs_debug, -1, 0, 0, 0, 0 }; ++ + static void *get_shm( unsigned int idx ) + { + int entry = (idx * 8) / pagesize; + int offset = (idx * 8) % pagesize; ++ void *ret; ++ ++ RtlEnterCriticalSection(&shm_addrs_section); + + if (entry >= shm_addrs_size) + { +@@ -180,7 +192,11 @@ static void *get_shm( unsigned int idx ) + munmap( addr, pagesize ); /* someone beat us to it */ + } + +- return (void *)((unsigned long)shm_addrs[entry] + offset); ++ ret = (void *)((unsigned long)shm_addrs[entry] + offset); ++ ++ RtlLeaveCriticalSection(&shm_addrs_section); ++ ++ return ret; + } + + /* We'd like lookup to be fast. To that end, we use a static list indexed by handle. +-- +2.23.0 + diff --git a/patches/eventfd_synchronization/0088-ntdll-Get-rid-of-the-per-event-spinlock-for-auto-res.patch b/patches/eventfd_synchronization/0088-ntdll-Get-rid-of-the-per-event-spinlock-for-auto-res.patch new file mode 100644 index 00000000..e8bf34e5 --- /dev/null +++ b/patches/eventfd_synchronization/0088-ntdll-Get-rid-of-the-per-event-spinlock-for-auto-res.patch @@ -0,0 +1,206 @@ +From c41dc5b8c422be3914cd31c239ec586a091b8a3b Mon Sep 17 00:00:00 2001 +From: Zebediah Figura +Date: Mon, 10 Jun 2019 11:25:34 -0400 +Subject: [PATCH] ntdll: Get rid of the per-event spinlock for auto-reset + events. + +It's not necessary. Much like semaphores, the shm state is just a hint. +--- + dlls/ntdll/esync.c | 74 +++++++++++++++++++++++++++++++++++----------- + server/esync.c | 32 +++++++++++++------- + 2 files changed, 78 insertions(+), 28 deletions(-) + +diff --git a/dlls/ntdll/esync.c b/dlls/ntdll/esync.c +index 2f030c141..87f303403 100644 +--- a/dlls/ntdll/esync.c ++++ b/dlls/ntdll/esync.c +@@ -570,6 +570,14 @@ static inline void small_pause(void) + * problem at all. + */ + ++/* Removing this spinlock is harder than it looks. esync_wait_objects() can ++ * deal with inconsistent state well enough, and a race between SetEvent() and ++ * ResetEvent() gives us license to yield either result as long as we act ++ * consistently, but that's not enough. Notably, esync_wait_objects() should ++ * probably act like a fence, so that the second half of esync_set_event() does ++ * not seep past a subsequent reset. That's one problem, but no guarantee there ++ * aren't others. */ ++ + NTSTATUS esync_set_event( HANDLE handle, LONG *prev ) + { + static const uint64_t value = 1; +@@ -583,21 +591,36 @@ NTSTATUS esync_set_event( HANDLE handle, LONG *prev ) + if ((ret = get_object( handle, &obj ))) return ret; + event = obj->shm; + +- /* Acquire the spinlock. */ +- while (interlocked_cmpxchg( &event->locked, 1, 0 )) +- small_pause(); ++ if (obj->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Acquire the spinlock. */ ++ while (interlocked_cmpxchg( &event->locked, 1, 0 )) ++ small_pause(); ++ } ++ ++ /* For manual-reset events, as long as we're in a lock, we can take the ++ * optimization of only calling write() if the event wasn't already ++ * signaled. ++ * ++ * For auto-reset events, esync_wait_objects() must grab the kernel object. ++ * Thus if we got into a race so that the shm state is signaled but the ++ * eventfd is unsignaled (i.e. reset shm, set shm, set fd, reset fd), we ++ * *must* signal the fd now, or any waiting threads will never wake up. */ + + /* Only bother signaling the fd if we weren't already signaled. */ +- if (!(current = interlocked_xchg( &event->signaled, 1 ))) ++ if (!(current = interlocked_xchg( &event->signaled, 1 )) || obj->type == ESYNC_AUTO_EVENT) + { + if (write( obj->fd, &value, sizeof(value) ) == -1) +- return FILE_GetNtStatus(); ++ ERR("write: %s\n", strerror(errno)); + } + + if (prev) *prev = current; + +- /* Release the spinlock. */ +- event->locked = 0; ++ if (obj->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Release the spinlock. */ ++ event->locked = 0; ++ } + + return STATUS_SUCCESS; + } +@@ -615,21 +638,34 @@ NTSTATUS esync_reset_event( HANDLE handle, LONG *prev ) + if ((ret = get_object( handle, &obj ))) return ret; + event = obj->shm; + +- /* Acquire the spinlock. */ +- while (interlocked_cmpxchg( &event->locked, 1, 0 )) +- small_pause(); ++ if (obj->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Acquire the spinlock. */ ++ while (interlocked_cmpxchg( &event->locked, 1, 0 )) ++ small_pause(); ++ } + +- /* Only bother signaling the fd if we weren't already signaled. */ +- if ((current = interlocked_xchg( &event->signaled, 0 ))) ++ /* For manual-reset events, as long as we're in a lock, we can take the ++ * optimization of only calling read() if the event was already signaled. ++ * ++ * For auto-reset events, we have no guarantee that the previous "signaled" ++ * state is actually correct. We need to leave both states unsignaled after ++ * leaving this function, so we always have to read(). */ ++ if ((current = interlocked_xchg( &event->signaled, 0 )) || obj->type == ESYNC_AUTO_EVENT) + { +- /* we don't care about the return value */ +- read( obj->fd, &value, sizeof(value) ); ++ if (read( obj->fd, &value, sizeof(value) ) == -1 && errno != EWOULDBLOCK && errno != EAGAIN) ++ { ++ ERR("read: %s\n", strerror(errno)); ++ } + } + + if (prev) *prev = current; + +- /* Release the spinlock. */ +- event->locked = 0; ++ if (obj->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Release the spinlock. */ ++ event->locked = 0; ++ } + + return STATUS_SUCCESS; + } +@@ -844,8 +880,9 @@ static void update_grabbed_object( struct esync *obj ) + else if (obj->type == ESYNC_AUTO_EVENT) + { + struct event *event = obj->shm; +- /* We don't have to worry about a race between this and read(), for +- * reasons described near esync_set_event(). */ ++ /* We don't have to worry about a race between this and read(), since ++ * this is just a hint, and the real state is in the kernel object. ++ * This might already be 0, but that's okay! */ + event->signaled = 0; + } + } +@@ -1094,6 +1131,7 @@ static NTSTATUS __esync_wait_objects( DWORD count, const HANDLE *handles, + } + else + { ++ /* FIXME: Could we check the poll or shm state first? Should we? */ + if ((size = read( fds[i].fd, &value, sizeof(value) )) == sizeof(value)) + { + /* We found our object. */ +diff --git a/server/esync.c b/server/esync.c +index 4521993d4..84d0951cb 100644 +--- a/server/esync.c ++++ b/server/esync.c +@@ -396,9 +396,12 @@ void esync_set_event( struct esync *esync ) + if (debug_level) + fprintf( stderr, "esync_set_event() fd=%d\n", esync->fd ); + +- /* Acquire the spinlock. */ +- while (interlocked_cmpxchg( &event->locked, 1, 0 )) +- small_pause(); ++ if (esync->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Acquire the spinlock. */ ++ while (interlocked_cmpxchg( &event->locked, 1, 0 )) ++ small_pause(); ++ } + + if (!interlocked_xchg( &event->signaled, 1 )) + { +@@ -406,8 +409,11 @@ void esync_set_event( struct esync *esync ) + perror( "esync: write" ); + } + +- /* Release the spinlock. */ +- event->locked = 0; ++ if (esync->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Release the spinlock. */ ++ event->locked = 0; ++ } + } + + void esync_reset_event( struct esync *esync ) +@@ -421,9 +427,12 @@ void esync_reset_event( struct esync *esync ) + if (debug_level) + fprintf( stderr, "esync_reset_event() fd=%d\n", esync->fd ); + +- /* Acquire the spinlock. */ +- while (interlocked_cmpxchg( &event->locked, 1, 0 )) +- small_pause(); ++ if (esync->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Acquire the spinlock. */ ++ while (interlocked_cmpxchg( &event->locked, 1, 0 )) ++ small_pause(); ++ } + + /* Only bother signaling the fd if we weren't already signaled. */ + if (interlocked_xchg( &event->signaled, 0 )) +@@ -432,8 +441,11 @@ void esync_reset_event( struct esync *esync ) + read( esync->fd, &value, sizeof(value) ); + } + +- /* Release the spinlock. */ +- event->locked = 0; ++ if (esync->type == ESYNC_MANUAL_EVENT) ++ { ++ /* Release the spinlock. */ ++ event->locked = 0; ++ } + } + + DECL_HANDLER(create_esync) +-- +2.23.0 + diff --git a/patches/patchinstall.sh b/patches/patchinstall.sh index f90193ef..7a0cfd14 100755 --- a/patches/patchinstall.sh +++ b/patches/patchinstall.sh @@ -3824,6 +3824,10 @@ if test "$enable_eventfd_synchronization" -eq 1; then patch_apply eventfd_synchronization/0082-ntdll-server-Check-the-value-of-WINEESYNC-instead-of.patch patch_apply eventfd_synchronization/0083-esync-Update-README.patch patch_apply eventfd_synchronization/0084-server-Use-default_fd_get_esync_fd-for-directory-cha.patch + patch_apply eventfd_synchronization/0085-server-Only-signal-the-APC-fd-for-user-APCs.patch + patch_apply eventfd_synchronization/0086-ntdll-Check-the-APC-fd-first.patch + patch_apply eventfd_synchronization/0087-ntdll-esync-Lock-accessing-the-shm_addrs-array.patch + patch_apply eventfd_synchronization/0088-ntdll-Get-rid-of-the-per-event-spinlock-for-auto-res.patch ( printf '%s\n' '+ { "Zebediah Figura", "configure: Check for sys/eventfd.h, ppoll(), and shm_open().", 1 },'; printf '%s\n' '+ { "Zebediah Figura", "server: Create server objects for eventfd-based synchronization objects.", 1 },'; @@ -3909,6 +3913,10 @@ if test "$enable_eventfd_synchronization" -eq 1; then printf '%s\n' '+ { "Zebediah Figura", "ntdll, server: Check the value of WINEESYNC instead of just the presence.", 1 },'; printf '%s\n' '+ { "Zebediah Figura", "esync: Update README.", 1 },'; printf '%s\n' '+ { "Zebediah Figura", "server: Create esync file descriptors for true file objects and use them for directory change notifications.", 1 },'; + printf '%s\n' '+ { "Zebediah Figura", "server: Only signal the APC fd for user APCs.", 1 },'; + printf '%s\n' '+ { "Zebediah Figura", "ntdll: Check the APC fd first.", 1 },'; + printf '%s\n' '+ { "Zebediah Figura", "ntdll/esync: Lock accessing the shm_addrs array.", 1 },'; + printf '%s\n' '+ { "Zebediah Figura", "ntdll: Get rid of the per-event spinlock for auto-reset events.", 1 },'; ) >> "$patchlist" fi