Intravision/reactos
1
0
Fork 0
mirror of https://github.com/reactos/reactos.git synced 2024-12-27 09:34:43 +00:00
Code Issues Projects Releases Packages Wiki Activity

[NTOS:CM] Lock the entire registry down when we unload a hive

Browse source 
The PR #6649 which fixed an issue with orphaned KCBs leaking in memory which also pointed to unloaded registry hives, it also brought a problem.
In CmpEnumerateOpenSubKeys there is a risk of getting hit by a deadlock as we enumerate the cache table to remove empty cache entries.

Fundamentally CmpEnumerateOpenSubKeys locks down a KCB from cache for exclusive use in order to tear down its contents from memory but it doesn't address the fact a KCB might have already been locked in the same calling thread, leading to a recursion.
This leads to random hangs when unloading a hive during system startup (tipically on a clean install).

The solution here is to simply lock the whole registry when we unload a hive so that we don't have to worry the KCBs are getting tampered by anybody else. This also simplifies the code.
Although locking the entire registry while other apps are doing registry related operations to other hives can cause overhead. If this turns out to be bad then we have to rethink the locking mechanism here.

CORE-19539
This commit is contained in:
George Bișoc 2024-05-26 21:05:37 +02:00
parent 5d0117de90
commit fe23a4aaeb
No known key found for this signature in database
GPG key ID: 688C4FBE25D7DEF6
3 changed files with 25 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
ntoskrnl/config/cmapi.c
View file

@ -2216,6 +2216,9 @@ CmUnloadKey(
DPRINT("CmUnloadKey(%p, %lx)\n", Kcb, Flags);
/* Ensure the registry is locked exclusively for the calling thread */
CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK();
/* Get the hive */
Hive = Kcb->KeyHive;
Cell = Kcb->KeyCell;
@ -2243,7 +2246,7 @@ CmUnloadKey(
{
if (Flags != REG_FORCE_UNLOAD)
{
if (CmpEnumerateOpenSubKeys(Kcb, FALSE, TRUE, FALSE) != 0)
if (CmpEnumerateOpenSubKeys(Kcb, TRUE, FALSE) != 0)
{
/* There are open subkeys but we don't force hive unloading, fail */
Hive->HiveFlags &= ~HIVE_IS_UNLOADING;
@ -2252,7 +2255,7 @@ CmUnloadKey(
}
else
{
if (CmpEnumerateOpenSubKeys(Kcb, TRUE, TRUE, TRUE) != 0)
if (CmpEnumerateOpenSubKeys(Kcb, TRUE, TRUE) != 0)
{
/* There are open subkeys that we cannot force to unload, fail */
Hive->HiveFlags &= ~HIVE_IS_UNLOADING;
@ -2293,14 +2296,8 @@ CmUnloadKey(
Kcb->Delete = TRUE;
CmpRemoveKeyControlBlock(Kcb);
if (Flags != REG_FORCE_UNLOAD)
{
/* Release the KCB locks */
CmpReleaseTwoKcbLockByKey(Kcb->ConvKey, Kcb->ParentKcb->ConvKey);
/* Release the hive loading lock */
ExReleasePushLockExclusive(&CmpLoadHiveLock);
}
/* Release the hive loading lock */
ExReleasePushLockExclusive(&CmpLoadHiveLock);
/* Release hive lock */
CmpUnlockRegistry();
@ -2341,7 +2338,6 @@ ULONG
NTAPI
CmpEnumerateOpenSubKeys(
_In_ PCM_KEY_CONTROL_BLOCK RootKcb,
_In_ BOOLEAN LockHeldExclusively,
_In_ BOOLEAN RemoveEmptyCacheEntries,
_In_ BOOLEAN DereferenceOpenedEntries)
{
@ -2354,6 +2350,9 @@ CmpEnumerateOpenSubKeys(
DPRINT("CmpEnumerateOpenSubKeys() called\n");
/* Ensure the registry is locked exclusively for the calling thread */
CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK();
/* The root key is the only referenced key. There are no referenced sub keys. */
if (RootKcb->RefCount == 1)
{
@ -2401,9 +2400,6 @@ CmpEnumerateOpenSubKeys(
if (DereferenceOpenedEntries &&
!(CachedKcb->ExtFlags & CM_KCB_READ_ONLY_KEY))
{
/* Registry needs to be locked down */
CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK();
/* Flush any notifications */
CmpFlushNotifiesOnKeyBodyList(CachedKcb, TRUE); // Lock is already held
@ -2431,20 +2427,11 @@ CmpEnumerateOpenSubKeys(
}
else if ((CachedKcb->RefCount == 0) && RemoveEmptyCacheEntries)
{
/* Lock the cached KCB of subkey before removing it from cache entries */
if (!LockHeldExclusively)
CmpAcquireKcbLockExclusive(CachedKcb);
/* Remove the current key from the delayed close list */
CmpRemoveFromDelayedClose(CachedKcb);
/* Remove the current cache entry */
// Lock is either held by ourselves or registry is locked exclusively
CmpCleanUpKcbCacheWithLock(CachedKcb, LockHeldExclusively);
/* Unlock the cached KCB if it was done by ourselves */
if (!LockHeldExclusively)
CmpReleaseKcbLock(CachedKcb);
CmpCleanUpKcbCacheWithLock(CachedKcb, TRUE);
/* Restart, because the hash list has changed */
Entry = CmpCacheTable[i].Entry;

64
ntoskrnl/config/ntapi.c
View file

@ -1565,7 +1565,7 @@ NtQueryOpenSubKeys(IN POBJECT_ATTRIBUTES TargetKey,
/* Call the internal API */
SubKeys = CmpEnumerateOpenSubKeys(KeyBody->KeyControlBlock,
TRUE, FALSE, FALSE);
FALSE, FALSE);
/* Unlock the registry */
CmpUnlockRegistry();
@ -1803,7 +1803,6 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey,
CM_PARSE_CONTEXT ParseContext = {0};
KPROCESSOR_MODE PreviousMode = ExGetPreviousMode();
PCM_KEY_BODY KeyBody = NULL;
ULONG ParentConv = 0, ChildConv = 0;
HANDLE Handle;
PAGED_CODE();
@ -1895,30 +1894,17 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey,
if (!NT_SUCCESS(Status))
return Status;
/* Acquire the lock depending on flags */
if (Flags == REG_FORCE_UNLOAD)
{
/* Lock registry exclusively */
CmpLockRegistryExclusive();
}
else
{
/* Lock registry */
CmpLockRegistry();
/* Acquire the hive loading lock */
ExAcquirePushLockExclusive(&CmpLoadHiveLock);
/* Lock parent and child */
if (KeyBody->KeyControlBlock->ParentKcb)
ParentConv = KeyBody->KeyControlBlock->ParentKcb->ConvKey;
else
ParentConv = KeyBody->KeyControlBlock->ConvKey;
ChildConv = KeyBody->KeyControlBlock->ConvKey;
CmpAcquireTwoKcbLocksExclusiveByKey(ChildConv, ParentConv);
}
/*
* Lock down the entire registry when we unload a hive.
*
* NOTE: We might block other threads of other processes that do
* operations with unrelated keys of other hives when we lock
* the registry for exclusive use by the calling thread that does
* the unloading. If this turns out to cause a major overhead we
* have to rethink the locking mechanism here (prior commit - f1d2a44).
*/
CmpLockRegistryExclusive();
ExAcquirePushLockExclusive(&CmpLoadHiveLock);
/* Check if it's being deleted already */
if (KeyBody->KeyControlBlock->Delete)
@ -1939,34 +1925,12 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey,
/* Call the internal API. Note that CmUnloadKey() unlocks the registry only on success. */
Status = CmUnloadKey(KeyBody->KeyControlBlock, Flags);
/* Check if we failed, but really need to succeed */
if ((Status == STATUS_CANNOT_DELETE) && (Flags == REG_FORCE_UNLOAD))
{
/* TODO: We should perform another attempt here */
_SEH2_TRY
{
DPRINT1("NtUnloadKey2(%wZ): We want to force-unload the hive but couldn't unload it: Retrying is UNIMPLEMENTED!\n", TargetKey->ObjectName);
}
_SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
{
}
_SEH2_END;
}
Quit:
/* If CmUnloadKey() failed we need to unlock registry ourselves */
if (!NT_SUCCESS(Status))
{
if (Flags != REG_FORCE_UNLOAD)
{
/* Release the KCB locks */
CmpReleaseTwoKcbLockByKey(ChildConv, ParentConv);
/* Release the hive loading lock */
ExReleasePushLockExclusive(&CmpLoadHiveLock);
}
/* Unlock the registry */
/* Unlock the hive loading and registry locks */
ExReleasePushLockExclusive(&CmpLoadHiveLock);
CmpUnlockRegistry();
}

1
ntoskrnl/include/internal/cm.h
View file

@ -1337,7 +1337,6 @@ ULONG
NTAPI
CmpEnumerateOpenSubKeys(
_In_ PCM_KEY_CONTROL_BLOCK RootKcb,
_In_ BOOLEAN LockHeldExclusively,
_In_ BOOLEAN RemoveEmptyCacheEntries,
_In_ BOOLEAN DereferenceOpenedEntries
);