[PATCH v2 0/1] MR1342: ntdll: Use an acquire/release pair on the IOSB status.
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 -- v2: ntdll: Use an acquire/release pair on the IOSB status. https://gitlab.winehq.org/wine/wine/-/merge_requests/1342
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- 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..ac04388acde 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..2db684ab7a0 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( &io->Status, status ); } else { IO_STATUS_BLOCK *io = wine_server_get_ptr( iosb ); + io->Information = info; #ifdef NONAMELESSUNION - io->u.Status = status; + WriteRelease( &io->u.Status, status ); #else - io->Status = status; + WriteRelease( &io->Status, status ); #endif - io->Information = info; } } diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 9e447f29d8c..09684f25b25 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1342
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126125 Your paranoid android. === debian11 (32 bit report) === crypt32: cert.c:4191: Test failed: success cert.c:4192: Test failed: got 00000000 cert.c:4193: Test failed: got 00000000
On Sun Nov 13 17:02:48 2022 +0000, Jinoh Kang wrote:
Why `(LONG *)`? (same for other accesses) I had forgotten what the actual definition of NTSTATUS was, turns out it is LONG, so removed in v2. The access to OVERLAPPED.Internal still needs the cast, though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15637
On Mon Nov 14 04:32:40 2022 +0000, Zebediah Figura wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/1342/diffs?diff_id=18421&start_sha=ac72e9c51cb2c31f3b2e089d6a4b8892ff63852b#6c845445f68f8d3b54ba3798c9bb9e781b28b2ec_3161_3161) Fixed in v2, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15639
On Mon Nov 14 04:32:44 2022 +0000, Jinoh Kang wrote:
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 sides 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! I probably never looked at that specific revision, and assumed from your private question that it still had no comments at all.
I did make a couple other modifications while I was at it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15640
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342
participants (4)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Zebediah Figura (＠zfigura)