[PATCH 0/1] MR1342: ntdll: Use an acquire/release pair on the IOSB status.

[Zebediah Figura (＠zfigura)]

Based on a patch by Jinoh Kang:

https://gitlab.winehq.org/wine/wine/-/merge_requests/155

This patch simplifies the comment somewhat; I don't see it as necessary to fully
explain the concept of memory barriers and acquire/release everywhere that they
are used. In my opinion, it's necessary and also sufficient to explain why these
specific places need an acquire/release pair, what that pair is protecting, and
to cross-reference the parts of the pair with each other.

I've also removed the IOSB aliasing patches. Personally I think that there's no
harm in making our code more endianness-conscious, for things like Winelib, but
on the other hand Windows never supported big-endian, and we've resisted adding
support for architectures Windows doesn't support (and see also [1]). Since it's
not clear to me they're desirable, and they're orthogonal to the purpose of this
patch set, I've removed them; they can be submitted later if there is a
favourable consensus.

[1] https://www.winehq.org/pipermail/wine-devel/2021-July/191600.html

[PATCH 1/1] ntdll: Use an acquire/release pair on the IOSB status.

[Jinoh Kang]

From: Jinoh Kang

---
 dlls/kernelbase/file.c         |  6 +++++-
 dlls/ntdll/unix/unix_private.h | 26 ++++++++++++++++++++++----
 dlls/ws2_32/socket.c           |  6 +++++-
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c
index 661bc0c2778..5ecf3049c4a 100644
--- a/dlls/kernelbase/file.c
+++ b/dlls/kernelbase/file.c
@@ -3156,7 +3156,9 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov

TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );

-    status = overlapped->Internal;
+    /* Paired with the write-release in set_async_iosb() in ntdll; see the
+     * latter for details. */
+    status = ReadAcquire( (LONG *)overlapped->Internal );
     if (status == STATUS_PENDING)
     {
         if (!timeout)
@@ -3173,6 +3175,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
             return FALSE;
         }

+        /* We don't need to give this load acquire semantics; the wait above
+         * already guarantees that the IOSB and output buffer are filled. */
         status = overlapped->Internal;
         if (status == STATUS_PENDING) status = STATUS_SUCCESS;
     }
diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h
index 73b9ed76de0..2ba5d1df295 100644
--- a/dlls/ntdll/unix/unix_private.h
+++ b/dlls/ntdll/unix/unix_private.h
@@ -363,6 +363,24 @@ static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR
 {
     if (!iosb) return;

+    /* GetOverlappedResult() and WSAGetOverlappedResult() expect that if the
+     * status is written, that the information (and buffer, which was written
+     * earlier from the async callback) will be available. Hence we need to
+     * store the status last, with release semantics to ensure that those
+     * writes are visible. This release is paired with a read-acquire in
+     * GetOverlappedResult() and WSAGetOverlappedResult():
+     *
+     * CPU 0 (set_async_iosb)            CPU 1 (GetOverlappedResultEx)
+     * ===========================       ===========================
+     * write buffer
+     * write Information
+     * WriteRelease(Status) <--------.
+     *                               |
+     *                               |
+     *                (paired with)  `-> ReadAcquire(Status)
+     *                                   read Information
+     */
+
     if (in_wow64_call())
     {
         struct iosb32
@@ -370,18 +388,18 @@ static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR
             NTSTATUS Status;
             ULONG    Information;
         } *io = wine_server_get_ptr( iosb );
-        io->Status = status;
         io->Information = info;
+        WriteRelease( (LONG *)&io->Status, status );
     }
     else
     {
         IO_STATUS_BLOCK *io = wine_server_get_ptr( iosb );
+        io->Information = info;
 #ifdef NONAMELESSUNION
-        io->u.Status = status;
+        WriteRelease( (LONG *)&io->u.Status, status );
 #else
-        io->Status = status;
+        WriteRelease( (LONG *)&io->Status, status );
 #endif
-        io->Information = info;
     }
 }

diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 9e447f29d8c..af7b45026ce 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -3710,7 +3710,9 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped,
         return FALSE;
     }

-    status = lpOverlapped->Internal;
+    /* Paired with the write-release in set_async_iosb() in ntdll; see the
+     * latter for details. */
+    status = ReadAcquire( (LONG *)lpOverlapped->Internal );
     if (status == STATUS_PENDING)
     {
         if (!fWait)
@@ -3722,6 +3724,8 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped,
         if (WaitForSingleObject( lpOverlapped->hEvent ? lpOverlapped->hEvent : SOCKET2HANDLE(s),
                                  INFINITE ) == WAIT_FAILED)
             return FALSE;
+        /* We don't need to give this load acquire semantics; the wait above
+         * already guarantees that the IOSB and output buffer are filled. */
         status = lpOverlapped->Internal;
     }

[PATCH 1/1] ntdll: Use an acquire/release pair on the IOSB status.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=126111

Your paranoid android.

=== debian11 (build log) ===

Error: Mount manager not running, most likely your WINEPREFIX wasn't created correctly.
Task: WineTest did not produce the win32 report

=== debian11b (build log) ===

Error: Mount manager not running, most likely your WINEPREFIX wasn't created correctly.
Task: WineTest did not produce the wow64 report

[PATCH 0/1] MR1342: ntdll: Use an acquire/release pair on the IOSB status. - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/kernelbase/file.c:
>  TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );
>
> - status = overlapped->Internal;
> - /* Paired with the write-release in set_async_iosb() in ntdll; see the
> -
> * latter for details. */
> - status = ReadAcquire( (LONG *)overlapped->Internal );

You forgot to take the address of `overlapped->Internal`.

```suggestion:-0+0
    status = ReadAcquire( (LONG *)&overlapped->Internal );
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/unix_private.h:
>          NTSTATUS Status;
>          ULONG    Information;
>      } *io = wine_server_get_ptr( iosb );
> -
>    io->Status = status;
>    io->Information = info;
> -
>    WriteRelease( (LONG *)&io->Status, status );

Why `(LONG *)`? (same for other accesses)

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ws2_32/socket.c:
>      return FALSE;
>  }
>
> - status = lpOverlapped->Internal;
> - /* Paired with the write-release in set_async_iosb() in ntdll; see the
> -
> * latter for details. */
> - status = ReadAcquire( (LONG *)lpOverlapped->Internal );

You forgot to take the address of `overlapped->Internal`.

```suggestion:-0+0
    status = ReadAcquire( (LONG *)&lpOverlapped->Internal );
```

[PATCH 0/1] MR1342: ntdll: Use an acquire/release pair on the IOSB status. - approved

[Jinoh Kang (＠iamahuman)]

This merge request was approved by Jinoh Kang.

[Jinoh Kang (＠iamahuman)]

So this basically brings us back to v9 of !155: https://gitlab.winehq.org/wine/wine/-/merge_requests/155/diffs?commit_id=354....

Note that this v9 patch was submitted *before* I eventually asked if we "need comments on read-acq side as well" on IRC; then, I interpreted your response "yes, both sides should have comments" in a way that requires both side to have detailed comments.

Perhaps I should have mentioned that I have made several modifications before asking there, so that you didn't have to work with outdated information.

Either way, thanks for your clarification!