[PATCH 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 -- 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..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; } -- 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 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
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342
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 ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15607
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) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15608
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 ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15609
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342
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! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1342#note_15610
participants (4)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Zebediah Figura (＠zfigura)