[ros-diffs] [fireball] 31262: - Use CmpLockRegistry instead of manual registry locking -- this also makes the registry lock shared instead of exclusive. - Start using KCB locking/unlocking, usually shared first. - Support KCB lock promotion from shared to exclusive when value cache functions require it (SearchNeedExclusiveLock -- not yet used). - Add multiple checks for deleted KCBs so that operations on them fail. - Protect symlinks from being erroneously modified. - Remove a ReactOS symlink hack. - Add co

Author: fireball
Date: Sun Dec 16 01:06:10 2007
New Revision: 31262

URL: http://svn.reactos.org/svn/reactos?rev=31262&view=rev
Log:
- Use CmpLockRegistry instead of manual registry locking -- this also makes the registry
lock shared instead of exclusive.
- Start using KCB locking/unlocking, usually shared first.
- Support KCB lock promotion from shared to exclusive when value cache functions require
it (SearchNeedExclusiveLock -- not yet used).
- Add multiple checks for deleted KCBs so that operations on them fail.
- Protect symlinks from being erroneously modified.
- Remove a ReactOS symlink hack.
- Add code to setup the KCB Value Cache (right now we use the node list since nobody was
setting the value cache up).
- Detect and assert on CM_KCB_SYM_LINK_FOUND, this is used by caching which we don't
support.
- Protect KEY_NO_DELETE nodes from deletion.
- Lock the registry during hive flushing.
- Respect CmpNoWrite if it's set.
- Don't ignore Allocate flag sent to CmpLinkhivetoMaster during hive loading.

Modified:
    trunk/reactos/ntoskrnl/config/cmapi.c

Modified: trunk/reactos/ntoskrnl/config/cmapi.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmapi.c?re…
==============================================================================
--- trunk/reactos/ntoskrnl/config/cmapi.c (original)
+++ trunk/reactos/ntoskrnl/config/cmapi.c Sun Dec 16 01:06:10 2007
@@ -266,10 +266,56 @@
     NTSTATUS Status;
     BOOLEAN Found, Result;
     ULONG Count, ChildIndex, SmallData, Storage;
-
-    /* Acquire hive lock exclusively */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    VALUE_SEARCH_RETURN_TYPE SearchResult;
+
+    /* Acquire hive lock */
+    CmpLockRegistry();
+    CmpAcquireKcbLockShared(Kcb);
+    
+    /* Sanity check */
+    ASSERT(sizeof(ULONG) == CM_KEY_VALUE_SMALL);
+    
+    /* Don't touch deleted KCBs */
+DoAgain:
+    if (Kcb->Delete)
+    {
+        /* Fail */
+        Status = STATUS_KEY_DELETED;
+        goto Quickie;
+    }
+    
+    /* Don't let anyone mess with symlinks */
+    if ((Kcb->Flags & KEY_SYM_LINK) &&
+        ((Type != REG_LINK) ||
+         !(ValueName) ||
+         !(RtlEqualUnicodeString(&CmSymbolicLinkValueName, ValueName, TRUE))))
+    {
+        /* Invalid modification of a symlink key */
+        Status = STATUS_ACCESS_DENIED;
+        goto Quickie;
+    }
+    
+    /* Search for the value */
+    SearchResult = SearchFail;
+    if (SearchResult == SearchNeedExclusiveLock)
+    {
+        /* Try again with the exclusive lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+        goto DoAgain;
+    }
+    else if (SearchResult == SearchSuccess)
+    {
+        /* We don't actually need to do anything! */
+        Status = STATUS_SUCCESS;
+        goto Quickie;
+    }
+
+    /* We need the exclusive KCB lock now */
+    if (!(CmpIsKcbLockedExclusive(Kcb)) &&
!(CmpTryToConvertKcbSharedToExclusive(Kcb)))
+    {
+        /* Acquire exclusive lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+    }
 
     /* Get pointer to key cell */
     Hive = Kcb->KeyHive;
@@ -315,7 +361,11 @@
         /* No child list, we'll need to add it */
         ChildIndex = 0;
     }
-
+    
+    /* The KCB must be locked exclusive at this point */
+    ASSERT((CmpIsKcbLockedExclusive(Kcb) == TRUE) ||
+           (CmpTestRegistryLockExclusive() == TRUE));
+    
     /* Mark the cell dirty */
     HvMarkCellDirty(Hive, Cell, FALSE);
 
@@ -357,27 +407,23 @@
                                         SmallData);
     }
 
-    /* Mark link key */
-    if ((Type == REG_LINK) &&
-        (_wcsicmp(ValueName->Buffer, L"SymbolicLinkValue") == 0))
-    {
-        Parent->Flags |= KEY_SYM_LINK;
-    }
-
     /* Check for success */
-Quickie:
     if (NT_SUCCESS(Status))
     {
+        /* Check if the maximum value name length changed */
         ASSERT(Parent->MaxValueNameLen == Kcb->KcbMaxValueNameLen);
         if (Parent->MaxValueNameLen < ValueName->Length)
         {
+            /* Set the new values */
             Parent->MaxValueNameLen = ValueName->Length;
             Kcb->KcbMaxValueNameLen = ValueName->Length;
         }
-        
+    
+        /* Check if the maximum data length changed */
         ASSERT(Parent->MaxValueDataLen == Kcb->KcbMaxValueDataLen);
         if (Parent->MaxValueDataLen < DataLength)
         {
+            /* Update it */
             Parent->MaxValueDataLen = DataLength;
             Kcb->KcbMaxValueDataLen = Parent->MaxValueDataLen;
         }
@@ -385,11 +431,32 @@
         /* Save the write time */
         KeQuerySystemTime(&Parent->LastWriteTime);
         KeQuerySystemTime(&Kcb->KcbLastWriteTime);
-    }
-
-    /* Release the lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+        
+        /* Check if the cell is cached */
+        if ((Found) && (CMP_IS_CELL_CACHED(Kcb->ValueCache.ValueList)))
+        {
+            /* Shouldn't happen */
+            ASSERT(FALSE);
+        }
+        else
+        {
+            /* Cleanup the value cache */
+            CmpCleanUpKcbValueCache(Kcb);
+
+            /* Sanity checks */
+            ASSERT(!(CMP_IS_CELL_CACHED(Kcb->ValueCache.ValueList)));
+            ASSERT(!(Kcb->ExtFlags & CM_KCB_SYM_LINK_FOUND));
+            
+            /* Set the value cache */
+            Kcb->ValueCache.Count = Parent->ValueList.Count;
+            Kcb->ValueCache.ValueList = Parent->ValueList.List;
+        }
+    }
+    
+Quickie:
+    /* Release the locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -408,8 +475,19 @@
     BOOLEAN Result;
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Lock KCB exclusively */
+    CmpAcquireKcbLockExclusive(Kcb);
+    
+    /* Don't touch deleted keys */
+    if (Kcb->Delete)
+    {
+        /* Undo everything */
+        CmpReleaseKcbLock(Kcb);
+        CmpUnlockRegistry();
+        return STATUS_KEY_DELETED;
+    }
 
     /* Get the hive and the cell index */
     Hive = Kcb->KeyHive;
@@ -490,6 +568,17 @@
             Kcb->KcbMaxValueNameLen = 0;
             Kcb->KcbMaxValueDataLen = 0;
         }
+        
+        /* Cleanup the value cache */
+        CmpCleanUpKcbValueCache(Kcb);
+        
+        /* Sanity checks */
+        ASSERT(!(CMP_IS_CELL_CACHED(Kcb->ValueCache.ValueList)));
+        ASSERT(!(Kcb->ExtFlags & CM_KCB_SYM_LINK_FOUND));
+        
+        /* Set the value cache */
+        Kcb->ValueCache.Count = ChildList->Count;
+        Kcb->ValueCache.ValueList = ChildList->List;
 
         /* Change default Status to success */
         Status = STATUS_SUCCESS;
@@ -507,9 +596,9 @@
         HvReleaseCell(Hive, ChildCell);
     }
 
-    /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    /* Release locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -533,8 +622,27 @@
     PAGED_CODE();
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Lock the KCB shared */
+    CmpAcquireKcbLockShared(Kcb);
+    
+    /* Don't touch deleted keys */
+DoAgain:
+    if (Kcb->Delete)
+    {
+        /* Undo everything */
+        CmpReleaseKcbLock(Kcb);
+        CmpUnlockRegistry();
+        return STATUS_KEY_DELETED;
+    }
+    
+    /* We don't deal with this yet */
+    if (Kcb->ExtFlags & CM_KCB_SYM_LINK_FOUND)
+    {
+        /* Shouldn't happen */
+        ASSERT(FALSE);
+    }
 
     /* Get the hive */
     Hive = Kcb->KeyHive;
@@ -547,6 +655,17 @@
                                          &ValueData,
                                          &ValueCached,
                                          &CellToRelease);
+    if (Result == SearchNeedExclusiveLock)
+    {
+        /* Check if we need an exclusive lock */
+        ASSERT(CellToRelease == HCELL_NIL);
+        ASSERT(ValueData == NULL);
+        
+        /* Try with exclusive KCB lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+        goto DoAgain;
+    }
+    
     if (Result == SearchSuccess)
     {
         /* Sanity check */
@@ -562,6 +681,12 @@
                                       Length,
                                       ResultLength,
                                       &Status);
+        if (Result == SearchNeedExclusiveLock)
+        {            
+            /* Try with exclusive KCB lock */
+            CmpConvertKcbSharedToExclusive(Kcb);
+            goto DoAgain;
+        }
     }
     else
     {
@@ -572,9 +697,9 @@
     /* If we have a cell to release, do so */
     if (CellToRelease != HCELL_NIL) HvReleaseCell(Hive, CellToRelease);
 
-    /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    /* Release locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -599,8 +724,20 @@
     PAGED_CODE();
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Lock the KCB shared */
+    CmpAcquireKcbLockShared(Kcb);
+
+    /* Don't touch deleted keys */
+DoAgain:
+    if (Kcb->Delete)
+    {
+        /* Undo everything */
+        CmpReleaseKcbLock(Kcb);
+        CmpUnlockRegistry();
+        return STATUS_KEY_DELETED;
+    }
 
     /* Get the hive and parent */
     Hive = Kcb->KeyHive;
@@ -620,6 +757,13 @@
         HvReleaseCell(Hive, Kcb->KeyCell);
         Status = STATUS_NO_MORE_ENTRIES;
         goto Quickie;
+    }
+    
+    /* We don't deal with this yet */
+    if (Kcb->ExtFlags & CM_KCB_SYM_LINK_FOUND)
+    {
+        /* Shouldn't happen */
+        ASSERT(FALSE);
     }
 
     /* Find the value list */
@@ -627,7 +771,17 @@
                                       &CellData,
                                       &IndexIsCached,
                                       &CellToRelease);
-    if (Result != SearchSuccess)
+    if (Result == SearchNeedExclusiveLock)
+    {
+        /* Check if we need an exclusive lock */
+        ASSERT(CellToRelease == HCELL_NIL);
+        ASSERT(ValueData == NULL);
+        
+        /* Try with exclusive KCB lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+        goto DoAgain;
+    }
+    else if (Result != SearchSuccess)
     {
         /* Sanity check */
         ASSERT(CellData == NULL);
@@ -646,10 +800,16 @@
                                      IndexIsCached,
                                      &ValueIsCached,
                                      &CellToRelease2);
-    if (Result != SearchSuccess)
+    if (Result == SearchNeedExclusiveLock)
+    {
+        /* Try with exclusive KCB lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+        goto DoAgain;
+    }
+    else if (Result != SearchSuccess)
     {
         /* Sanity check */
-        ASSERT(CellToRelease2 == HCELL_NIL);
+        ASSERT(ValueData == NULL);
 
         /* Release the cells and fail */
         Status = STATUS_INSUFFICIENT_RESOURCES;
@@ -666,6 +826,12 @@
                                   Length,
                                   ResultLength,
                                   &Status);
+    if (Result == SearchNeedExclusiveLock)
+    {
+        /* Try with exclusive KCB lock */
+        CmpConvertKcbSharedToExclusive(Kcb);
+        goto DoAgain;
+    }
 
 Quickie:
     /* If we have a cell to release, do so */
@@ -677,9 +843,9 @@
     /* If we have a cell to release, do so */
     if (CellToRelease2 != HCELL_NIL) HvReleaseCell(Hive, CellToRelease2);
 
-    /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    /* Release locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -911,8 +1077,10 @@
     PCM_KEY_NODE Parent;
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Lock KCB shared */
+    CmpAcquireKcbLockShared(Kcb);
 
     /* Get the hive and parent */
     Hive = Kcb->KeyHive;
@@ -921,6 +1089,14 @@
     {
         /* Fail */
         Status = STATUS_INSUFFICIENT_RESOURCES;
+        goto Quickie;
+    }
+    
+    /* Don't touch deleted keys */
+    if (Kcb->Delete)
+    {
+        /* Fail */
+        Status = STATUS_KEY_DELETED;
         goto Quickie;
     }
 
@@ -961,9 +1137,9 @@
     }
 
 Quickie:
-    /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    /* Release locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -982,8 +1158,19 @@
     HCELL_INDEX ChildCell;
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Lock the KCB shared */
+    CmpAcquireKcbLockShared(Kcb);
+    
+    /* Don't touch deleted keys */
+    if (Kcb->Delete)
+    {
+        /* Undo everything */
+        CmpReleaseKcbLock(Kcb);
+        CmpUnlockRegistry();
+        return STATUS_KEY_DELETED;
+    }
 
     /* Get the hive and parent */
     Hive = Kcb->KeyHive;
@@ -1027,9 +1214,9 @@
                              ResultLength);
 
 Quickie:
-    /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    /* Release locks */
+    CmpReleaseKcbLock(Kcb);
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -1041,11 +1228,32 @@
     PHHIVE Hive;
     PCM_KEY_NODE Node, Parent;
     HCELL_INDEX Cell, ParentCell;
-    PCM_KEY_CONTROL_BLOCK Kcb = KeyBody->KeyControlBlock;
+    PCM_KEY_CONTROL_BLOCK Kcb;
 
     /* Acquire hive lock */
-    KeEnterCriticalRegion();
-    ExAcquireResourceExclusiveLite(&CmpRegistryLock, TRUE);
+    CmpLockRegistry();
+    
+    /* Get the kcb */
+    Kcb = KeyBody->KeyControlBlock;
+    
+    /* Don't allow deleting the root */
+    if (!Kcb->ParentKcb)
+    {
+        /* Fail */
+        CmpUnlockRegistry();
+        return STATUS_CANNOT_DELETE;
+    }
+    
+    /* Lock parent and child */
+    CmpAcquireTwoKcbLocksExclusiveByKey(Kcb->ConvKey, Kcb->ParentKcb->ConvKey);
+    
+    /* Check if we're already being deleted */
+    if (Kcb->Delete)
+    {
+        /* Don't do it twice */
+        Status = STATUS_SUCCESS;
+        goto Quickie2;
+    }
 
     /* Get the hive and node */
     Hive = Kcb->KeyHive;
@@ -1059,34 +1267,22 @@
         Status = STATUS_INSUFFICIENT_RESOURCES;
         goto Quickie;
     }
-
-    /* Check if we have no parent */
-    if (!Node->Parent)
-    {
-        /* This is an attempt to delete \Registry itself! */
-        Status = STATUS_CANNOT_DELETE;
-        goto Quickie;
-    }
-    
-    /* Check if we're already being deleted */
-    if (Kcb->Delete)
-    {
-        /* Don't do it twice */
-        Status = STATUS_SUCCESS;
-        goto Quickie;
-    }
-    
+   
     /* Sanity check */
     ASSERT(Node->Flags == Kcb->Flags);
 
     /* Check if we don't have any children */
-    if (!(Node->SubKeyCounts[Stable] + Node->SubKeyCounts[Volatile]))
+    if (!(Node->SubKeyCounts[Stable] + Node->SubKeyCounts[Volatile]) &&
+        !(Node->Flags & KEY_NO_DELETE))
     {
         /* Get the parent and free the cell */
         ParentCell = Node->Parent;
         Status = CmpFreeKeyByCell(Hive, Cell, TRUE);
         if (NT_SUCCESS(Status))
         {
+            /* Clean up information we have on the subkey */
+            CmpCleanUpSubKeyInfo(Kcb->ParentKcb);
+
             /* Get the parent node */
             Parent = (PCM_KEY_NODE)HvGetCell(Hive, ParentCell);
             if (Parent)
@@ -1119,16 +1315,16 @@
         Status = STATUS_CANNOT_DELETE;
     }
     
-    /* Flush the registry */
-    CmpLazyFlush();
-    
 Quickie:
     /* Release the cell */
     HvReleaseCell(Hive, Cell);
+    
+    /* Release the KCB locks */
+Quickie2:
+    CmpReleaseTwoKcbLockByKey(Kcb->ConvKey, Kcb->ParentKcb->ConvKey);
 
     /* Release hive lock */
-    ExReleaseResourceLite(&CmpRegistryLock);
-    KeLeaveCriticalRegion();
+    CmpUnlockRegistry();
     return Status;
 }
 
@@ -1140,6 +1336,9 @@
     PCMHIVE CmHive;
     NTSTATUS Status = STATUS_SUCCESS;
     PHHIVE Hive;
+
+    /* Ignore flushes until we're ready */
+    if (CmpNoWrite) return STATUS_SUCCESS;
     
     /* Get the hives */
     Hive = Kcb->KeyHive;
@@ -1239,7 +1438,7 @@
     }
     
     /* Lock the registry shared */
-    //CmpLockRegistry();
+    CmpLockRegistry();
     
     /* Lock the hive to this thread */
     CmHive->Hive.HiveFlags |= HIVE_IS_UNLOADING;
@@ -1252,7 +1451,7 @@
     Status = CmpLinkHiveToMaster(TargetKey->ObjectName,
                                  TargetKey->RootDirectory,
                                  CmHive,
-                                 FALSE,
+                                 Allocate,
                                  TargetKey->SecurityDescriptor);
     if (NT_SUCCESS(Status))
     {
@@ -1276,7 +1475,7 @@
     }
     
     /* Unlock the registry */
-    //CmpUnlockRegistry();
+    CmpUnlockRegistry();
     
     /* Close handle and return */
     if (KeyHandle) ZwClose(KeyHandle);