[ros-dev] [ros-diffs] [gedmurphy] 70408: [NTOSKRNL] - Raise the IRQL when enumerating device lists so it doesn't get edited mid-listing - Don't hardcode the pointer size when checking the buffer size

[Hermès BÉLUSCA - MAÏTO]

Also in NT6+, the kernel seems to always be multiprocessor. And yes, the KeAcquire(queued)SpinLock becomes KeRaiseIrql in a SP kernel. Finally, directly using the correct function (AcquireSpinLock) would be better to know have to hack again the kernel to add support for MP afterwards. Better do it correctly now.

-----Message d'origine-----
De : Hermès BÉLUSCA - MAÏTO [mailto:hermes.belusca at sfr.fr] 
Envoyé : mercredi 30 décembre 2015 11:16
À : 'ReactOS Development List'
Objet : RE: [ros-dev] [ros-diffs] [gedmurphy] 70408: [NTOSKRNL] - Raise the IRQL when enumerating device lists so it doesn't get edited mid-listing - Don't hardcode the pointer size when checking the buffer size

To answer Ged's worry, I don't think here people would get angry, since that spinlock is also used there: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=LockQueueIoDatabaseLock (see iomgr/file.c and volume.c) .

-----Message d'origine-----
De : Ros-dev [mailto:ros-dev-bounces at reactos.org] De la part de Thomas Faber Envoyé : mardi 29 décembre 2015 22:07 À : ReactOS Development List Objet : Re: [ros-dev] [ros-diffs] [gedmurphy] 70408: [NTOSKRNL] - Raise the IRQL when enumerating device lists so it doesn't get edited mid-listing - Don't hardcode the pointer size when checking the buffer size

I think this might be an SP/MP difference?
Would make sense if KeAcquireSpinLock inside an SP kernel becomes KeRaiseIrql. But even for a 2003 MP kernel this would serve no purpose.


On 2015-12-29 21:48, Ged Murphy wrote:
> It depends on whether we want to emulate the 2k3 kernel or not.
> 
> I noticed in my 2k3 fltmgr testing that IoEnumerateDeviceObjectList 
> runs at DPC for the entirety of the function. This was changed in the
> NT6 kernel to use a lock, which is indeed the LockQueueIoDatabaseLock Hermes mentioned.
> 
> I'm happy to use the spinlock (and would prefer to myself), but any 
> move towards NT6 normally results in an angry email, so I opted for 
> the 2k3 approach  ;)
> 
> Ged.
> 
> 
> -----Original Message-----
> From: Ros-dev [mailto:ros-dev-bounces at reactos.org] On Behalf Of Hermès 
> BÉLUSCA - MAÏTO
> Sent: 29 December 2015 16:08
> To: 'ReactOS Development List' <ros-dev at reactos.org>
> Subject: Re: [ros-dev] [ros-diffs] [gedmurphy] 70408: [NTOSKRNL] - 
> Raise the IRQL when enumerating device lists so it doesn't get edited 
> mid-listing - Don't hardcode the pointer size when checking the buffer 
> size
> 
> Yes this should be a spinlock involved instead, for example the 
> "LockQueueIoDatabaseLock" (queued) spinlock that we already use in 
> other places in the code.
> 
> -----Message d'origine-----
> De : Ros-dev [mailto:ros-dev-bounces at reactos.org] De la part de Thomas 
> Faber Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev at reactos.org Objet : Re:
> [ros-dev] [ros-diffs] [gedmurphy] 70408: [NTOSKRNL] - Raise the IRQL 
> when enumerating device lists so it doesn't get edited mid-listing - 
> Don't hardcode the pointer size when checking the buffer size
> 
> Uhm... raising the IRQL is not a synchronization mechanism. Should 
> there be a spinlock involved?
> 
> 
> On 2015-12-23 12:26, gedmurphy at svn.reactos.org wrote:
>> Author: gedmurphy
>> Date: Wed Dec 23 11:26:28 2015
>> New Revision: 70408
>>
>> URL: http://svn.reactos.org/svn/reactos?rev=70408&view=rev
>> Log:
>> [NTOSKRNL]
>> - Raise the IRQL when enumerating device lists so it doesn't get 
>> edited mid-listing
>> - Don't hardcode the pointer size when checking the buffer size
>>
>> Modified:
>>     trunk/reactos/ntoskrnl/io/iomgr/device.c
>>
>> Modified: trunk/reactos/ntoskrnl/io/iomgr/device.c
>> URL: 
>> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/de
>> v ice.c?rev=70408&r1=70407&r2=70408&view=diff
>>
> ======================================================================
> ======
> ==
>> --- trunk/reactos/ntoskrnl/io/iomgr/device.c	[iso-8859-1] (original)
>> +++ trunk/reactos/ntoskrnl/io/iomgr/device.c	[iso-8859-1] Wed Dec 23
> 11:26:28 2015
>> @@ -1088,6 +1088,10 @@
>>  {
>>      ULONG ActualDevices = 1;
>>      PDEVICE_OBJECT CurrentDevice = DriverObject->DeviceObject;
>> +    KIRQL OldIrql;
>> +
>> +    /* Raise to dispatch level */
>> +    KeRaiseIrql(DISPATCH_LEVEL, &OldIrql);
>>  
>>      /* Find out how many devices we'll enumerate */
>>      while ((CurrentDevice = CurrentDevice->NextDevice))
>> ActualDevices++; @@ -1099,13 +1103,14 @@
>>      *ActualNumberDeviceObjects = ActualDevices;
>>  
>>      /* Check if we can support so many */
>> -    if ((ActualDevices * 4) > DeviceObjectListSize)
>> +    if ((ActualDevices * sizeof(PDEVICE_OBJECT)) >
>> + DeviceObjectListSize)
>>      {
>>          /* Fail because the buffer was too small */
>> +        KeLowerIrql(OldIrql);
>>          return STATUS_BUFFER_TOO_SMALL;
>>      }
>>  
>> -    /* Check if the caller only wanted the size */
>> +    /* Check if the caller wanted the device list */
>>      if (DeviceObjectList)
>>      {
>>          /* Loop through all the devices */ @@ -1123,6 +1128,9 @@
>>              DeviceObjectList++;
>>          }
>>      }
>> +
>> +    /* Return back to previous IRQL */
>> +    KeLowerIrql(OldIrql);
>>  
>>      /* Return the status */
>>      return STATUS_SUCCESS;
>>
>>


_______________________________________________
Ros-dev mailing list
Ros-dev at reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev