[PATCH v5 0/11] MR8995: Draft: ntdll/kernelbase: Add accurate node count feature to prevent memory access violations initializing multiple physical nodes as a single logical one on multi-node systems
A much cleaner result (written by hand) that we can reuse where needed later (_i.e._ `GetNumaProcessorNode` which also crashes with [a similar MAV](https://gist.github.com/wasertech/f894ce8d6250e72a01a861c0e4eb6064) on multi-node systems). Took me a while to understand where I should put everything, not even sure I got it right. Let me know. I thought I was going to need `FileNumaNodeInformation` but turns out it's not really needed. I can remove it if you want. This is what I see when I try to get a node count read: ```log 0024:fixme:ntdll:init_numa_info node affinity; using node 0. GetNumaHighestNodeNumber: 1 ``` I only see this fixme if I try to access `FILE_NUMA_NODE_INFORMATION`. I would really prefer such a behavior for our compatibility layer on any multi-node system. A big thanks to @besentv and @zfigura for their invaluable feedback on this. -- v5: ntdll: simplify return handling from `init_numa_info` in `NtQuerySystemInformationEx( SystemNumaProcessorMap )` ntdll: refactor init_numa_info to return NTSTATUS and improve error handling ntdll: remove unused definition https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/kernelbase/memory.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index c0a3d65d341..dfa4c434d2b 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -1582,8 +1582,46 @@ BOOL WINAPI SetProcessDefaultCpuSets(HANDLE process, const ULONG *cpu_set_ids, U */ BOOL WINAPI DECLSPEC_HOTPATCH GetNumaHighestNodeNumber( ULONG *node ) { - FIXME( "semi-stub: %p\n", node ); + NTSTATUS nts; + DWORD rel = RelationNumaNode; + DWORD iex_len = 0; + SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *iex = NULL; + unsigned int i; + + if (!node) { SetLastError( ERROR_INVALID_PARAMETER ); return FALSE; } *node = 0; + + set_ntstatus( NtQuerySystemInformationEx( SystemLogicalProcessorInformationEx, + &rel, sizeof(rel), NULL, 0, &iex_len ) ); + if (!iex_len) + { FIXME( "stub: iex_len %lu, node %lu\n", iex_len, *node ); return TRUE; } + + iex = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex_len ); + if (!iex) { FIXME( "stub: iex %p, node %lu\n", iex, *node ); return TRUE; } + + nts = set_ntstatus( NtQuerySystemInformationEx( SystemLogicalProcessorInformationEx, + &rel, sizeof(rel), iex, iex_len, &iex_len ) ); + if (!nts) + { + FIXME( "stub: nts %lu, node %lu\n", nts, *node ); + RtlFreeHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex ); + return TRUE; + } + + for ( i = 0; i < iex_len; ) + { + SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *ex = ( SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX * ) + ( (char *)iex + i ); + if (!ex->Size) break; + if (ex->Relationship == RelationNumaNode) + { + if (ex->NumaNode.NodeNumber > *node) + *node = ex->NumaNode.NodeNumber; + } + i += ex->Size; + } + + RtlFreeHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex ); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/kernel32/tests/process.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 7984f3d6fb4..fe2644c73e1 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -79,6 +79,7 @@ static BOOL (WINAPI *pQueryInformationJobObject)(HANDLE job, JOBOBJECTINFOCLAS static BOOL (WINAPI *pSetInformationJobObject)(HANDLE job, JOBOBJECTINFOCLASS class, LPVOID info, DWORD len); static HANDLE (WINAPI *pCreateIoCompletionPort)(HANDLE file, HANDLE existing_port, ULONG_PTR key, DWORD threads); static BOOL (WINAPI *pGetNumaProcessorNode)(UCHAR, PUCHAR); +static BOOL (WINAPI *pGetNumaHighestNodeNumber)(ULONG); static NTSTATUS (WINAPI *pNtQueryInformationProcess)(HANDLE, PROCESSINFOCLASS, PVOID, ULONG, PULONG); static NTSTATUS (WINAPI *pNtQueryInformationThread)(HANDLE, THREADINFOCLASS, PVOID, ULONG, PULONG); static NTSTATUS (WINAPI *pNtQuerySystemInformationEx)(SYSTEM_INFORMATION_CLASS, void*, ULONG, void*, ULONG, ULONG*); @@ -269,6 +270,7 @@ static BOOL init(void) pSetInformationJobObject = (void *)GetProcAddress(hkernel32, "SetInformationJobObject"); pCreateIoCompletionPort = (void *)GetProcAddress(hkernel32, "CreateIoCompletionPort"); pGetNumaProcessorNode = (void *)GetProcAddress(hkernel32, "GetNumaProcessorNode"); + pGetNumaHighestNodeNumber = (void *)GetProcAddress(hkernel32, "GetNumaHighestNodeNumber"); pWTSGetActiveConsoleSessionId = (void *)GetProcAddress(hkernel32, "WTSGetActiveConsoleSessionId"); pCreateToolhelp32Snapshot = (void *)GetProcAddress(hkernel32, "CreateToolhelp32Snapshot"); pProcess32First = (void *)GetProcAddress(hkernel32, "Process32First"); @@ -4087,6 +4089,24 @@ static void test_GetNumaProcessorNode(void) } } +static void test_GetNumaHighestNodeNumber(void) +{ + BOOL could_find_nodes; + ULONG node_count; + + if (!pGetNumaHighestNodeNumber) + { + win_skip("GetNumaHighestNodeNumber is missing\n"); + return; + } + + node_count = 0xFF; + could_find_nodes = pGetNumaHighestNodeNumber(&node_count); + ok(could_find_nodes, "GetNumaHighestNodeNumber returned FALSE\n"); + ok(node_count <= 64 && node_count != 0xFF && node_count >= 0, + "GetNumaHighestNodeNumber returned invalid node %u\n", node_count); +} + static void test_session_info(void) { DWORD session_id, active_session; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/file.c | 8 +++++++- dlls/ntdll/unix/system.c | 41 ++++++++++++++++++++++++++++++++++++++++ include/winternl.h | 6 ++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 958ae9a6937..13d4cfafd8c 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -125,6 +125,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(file); WINE_DECLARE_DEBUG_CHANNEL(winediag); +BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info); + #define MAX_DOS_DRIVES 26 /* just in case... */ @@ -4583,7 +4585,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, 0, /* FileIdGlobalTxDirectoryInformation */ 0, /* FileIsRemoteDeviceInformation */ 0, /* FileAttributeCacheInformation */ - 0, /* FileNumaNodeInformation */ + sizeof(FILE_NUMA_NODE_INFORMATION), /* FileNumaNodeInformation */ 0, /* FileStandardLinkInformation */ 0, /* FileRemoteProtocolInformation */ 0, /* FileRenameInformationBypassAccessCheck */ @@ -4735,6 +4737,10 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, info->ReparseTag = IO_REPARSE_TAG_MOUNT_POINT; } break; + case FileNumaNodeInformation: + FILE_NUMA_NODE_INFORMATION *ni = (FILE_NUMA_NODE_INFORMATION *)ptr; + if (!init_numa_info(ni)) status = STATUS_NOT_IMPLEMENTED; + status = STATUS_SUCCESS; break; case FileStatInformation: if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index 342e443f5e7..bf61f179849 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -1404,6 +1404,33 @@ static void init_logical_proc_info(void) init_tsc_frequency(); } +BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info) +{ + unsigned int i; + + if (!info) return FALSE; + pthread_once(&logical_proc_init_once, init_logical_proc_info); + + info->HighestNodeNumber = 0; + info->NodeNumber = 0; + info->Reserved = 0; + + if (!logical_proc_info_len || !logical_proc_info) + return TRUE; + + for (i = 0; i < logical_proc_info_len; ++i) + { + if (logical_proc_info[i].Relationship == RelationNumaNode) + { + if (logical_proc_info[i].NumaNode.NodeNumber > info->HighestNodeNumber) + info->HighestNodeNumber = logical_proc_info[i].NumaNode.NodeNumber; + } + } + + info->NodeNumber = 0; + return TRUE; +} + /****************************************************************** * init_cpu_info * @@ -3885,6 +3912,20 @@ NTSTATUS WINAPI NtQuerySystemInformationEx( SYSTEM_INFORMATION_CLASS class, break; } + case SystemNumaProximityNodeInformation: + { + FILE_NUMA_NODE_INFORMATION *numa_info = info; + + len = sizeof(FILE_NUMA_NODE_INFORMATION); + if (size < len) { ret = STATUS_BUFFER_TOO_SMALL; break; } + if (!info) return STATUS_ACCESS_VIOLATION; + if (!init_numa_info(numa_info)) + ret = STATUS_NOT_IMPLEMENTED; + + ret = STATUS_SUCCESS; + break; + } + default: FIXME( "(0x%08x,%p,%u,%p,%u,%p) stub\n", class, query, query_len, info, size, ret_size ); break; diff --git a/include/winternl.h b/include/winternl.h index 36c24b01e56..39e720554db 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -1790,6 +1790,12 @@ typedef struct _FILE_STAT_INFORMATION { ULONG EffectiveAccess; } FILE_STAT_INFORMATION, *PFILE_STAT_INFORMATION; +typedef struct _FILE_NUMA_NODE_INFORMATION { + ULONG HighestNodeNumber; + ULONG NodeNumber; + ULONG Reserved; +} FILE_NUMA_NODE_INFORMATION, *PFILE_NUMA_NODE_INFORMATION; + typedef struct _FILE_IO_COMPLETION_NOTIFICATION_INFORMATION { ULONG Flags; } FILE_IO_COMPLETION_NOTIFICATION_INFORMATION, *PFILE_IO_COMPLETION_NOTIFICATION_INFORMATION; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/kernelbase/memory.c | 43 +++++++--------------------------------- 1 file changed, 7 insertions(+), 36 deletions(-) diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index dfa4c434d2b..d1f828114dd 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -1582,46 +1582,17 @@ BOOL WINAPI SetProcessDefaultCpuSets(HANDLE process, const ULONG *cpu_set_ids, U */ BOOL WINAPI DECLSPEC_HOTPATCH GetNumaHighestNodeNumber( ULONG *node ) { - NTSTATUS nts; - DWORD rel = RelationNumaNode; - DWORD iex_len = 0; - SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *iex = NULL; - unsigned int i; + FILE_NUMA_NODE_INFORMATION ni; if (!node) { SetLastError( ERROR_INVALID_PARAMETER ); return FALSE; } - *node = 0; - - set_ntstatus( NtQuerySystemInformationEx( SystemLogicalProcessorInformationEx, - &rel, sizeof(rel), NULL, 0, &iex_len ) ); - if (!iex_len) - { FIXME( "stub: iex_len %lu, node %lu\n", iex_len, *node ); return TRUE; } - - iex = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex_len ); - if (!iex) { FIXME( "stub: iex %p, node %lu\n", iex, *node ); return TRUE; } - - nts = set_ntstatus( NtQuerySystemInformationEx( SystemLogicalProcessorInformationEx, - &rel, sizeof(rel), iex, iex_len, &iex_len ) ); - if (!nts) - { - FIXME( "stub: nts %lu, node %lu\n", nts, *node ); - RtlFreeHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex ); - return TRUE; - } - for ( i = 0; i < iex_len; ) - { - SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *ex = ( SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX * ) - ( (char *)iex + i ); - if (!ex->Size) break; - if (ex->Relationship == RelationNumaNode) - { - if (ex->NumaNode.NodeNumber > *node) - *node = ex->NumaNode.NodeNumber; - } - i += ex->Size; - } + if ( + !set_ntstatus( NtQuerySystemInformationEx( SystemNumaProximityNodeInformation, + NULL, 0, &ni, sizeof(ni), NULL )) + ) + return FALSE; - RtlFreeHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, iex ); + *node = ni.HighestNodeNumber; return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/system.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index bf61f179849..2b5ea75480f 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -1428,6 +1428,7 @@ BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info) } info->NodeNumber = 0; + FIXME( "node affinity; using node %u.\n", info->NodeNumber ); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/system.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index 2b5ea75480f..ac06ebb362b 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -3913,7 +3913,7 @@ NTSTATUS WINAPI NtQuerySystemInformationEx( SYSTEM_INFORMATION_CLASS class, break; } - case SystemNumaProximityNodeInformation: + case SystemNumaProcessorMap: { FILE_NUMA_NODE_INFORMATION *numa_info = info; @@ -3922,8 +3922,8 @@ NTSTATUS WINAPI NtQuerySystemInformationEx( SYSTEM_INFORMATION_CLASS class, if (!info) return STATUS_ACCESS_VIOLATION; if (!init_numa_info(numa_info)) ret = STATUS_NOT_IMPLEMENTED; - - ret = STATUS_SUCCESS; + else + ret = STATUS_SUCCESS; break; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/kernelbase/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c index d1f828114dd..58f53fdf11c 100644 --- a/dlls/kernelbase/memory.c +++ b/dlls/kernelbase/memory.c @@ -1587,7 +1587,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetNumaHighestNodeNumber( ULONG *node ) if (!node) { SetLastError( ERROR_INVALID_PARAMETER ); return FALSE; } if ( - !set_ntstatus( NtQuerySystemInformationEx( SystemNumaProximityNodeInformation, + !set_ntstatus( NtQuerySystemInformationEx( SystemNumaProcessorMap, NULL, 0, &ni, sizeof(ni), NULL )) ) return FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/system.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index ac06ebb362b..f6ba50fcb48 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -1428,7 +1428,7 @@ BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info) } info->NodeNumber = 0; - FIXME( "node affinity; using node %u.\n", info->NodeNumber ); + if (info->HighestNodeNumber > info->NodeNumber) { FIXME( "node affinity; using only node %u.\n", info->NodeNumber ); } return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/file.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 13d4cfafd8c..93029f16ee2 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -125,8 +125,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(file); WINE_DECLARE_DEBUG_CHANNEL(winediag); -BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info); - #define MAX_DOS_DRIVES 26 /* just in case... */ @@ -4737,10 +4735,6 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, info->ReparseTag = IO_REPARSE_TAG_MOUNT_POINT; } break; - case FileNumaNodeInformation: - FILE_NUMA_NODE_INFORMATION *ni = (FILE_NUMA_NODE_INFORMATION *)ptr; - if (!init_numa_info(ni)) status = STATUS_NOT_IMPLEMENTED; - status = STATUS_SUCCESS; break; case FileStatInformation: if (fd_get_file_info( fd, options, &st, &attr ) == -1) status = errno_to_status( errno ); else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/system.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index f6ba50fcb48..654f421324a 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -1404,19 +1404,20 @@ static void init_logical_proc_info(void) init_tsc_frequency(); } -BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info) +NTSTATUS init_numa_info(FILE_NUMA_NODE_INFORMATION *info) { unsigned int i; - if (!info) return FALSE; - pthread_once(&logical_proc_init_once, init_logical_proc_info); + if (!info) { return STATUS_INVALID_PARAMETER; } + if (pthread_once(&logical_proc_init_once, init_logical_proc_info) != 0) { + return STATUS_UNSUCCESSFUL; + } info->HighestNodeNumber = 0; info->NodeNumber = 0; info->Reserved = 0; - if (!logical_proc_info_len || !logical_proc_info) - return TRUE; + if (logical_proc_info_len > 0 && !logical_proc_info) { return STATUS_UNSUCCESSFUL; } for (i = 0; i < logical_proc_info_len; ++i) { @@ -1429,7 +1430,7 @@ BOOL init_numa_info(FILE_NUMA_NODE_INFORMATION *info) info->NodeNumber = 0; if (info->HighestNodeNumber > info->NodeNumber) { FIXME( "node affinity; using only node %u.\n", info->NodeNumber ); } - return TRUE; + return STATUS_SUCCESS; } /****************************************************************** -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
From: wasertech <danny(a)waser.tech> --- dlls/ntdll/unix/system.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index 654f421324a..7e5cbbf9cb2 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -3921,10 +3921,7 @@ NTSTATUS WINAPI NtQuerySystemInformationEx( SYSTEM_INFORMATION_CLASS class, len = sizeof(FILE_NUMA_NODE_INFORMATION); if (size < len) { ret = STATUS_BUFFER_TOO_SMALL; break; } if (!info) return STATUS_ACCESS_VIOLATION; - if (!init_numa_info(numa_info)) - ret = STATUS_NOT_IMPLEMENTED; - else - ret = STATUS_SUCCESS; + ret = init_numa_info(numa_info); break; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8995
participants (2)
-
Danny Waser (＠wasertech) -
wasertech