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

[Thomas Faber]

Uhm... raising the IRQL is not a synchronization mechanism. Should
there be a spinlock involved?


On 2015-12-23 12:26, gedmurphy(a)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/device.c…
>  ==============================================================================
>  --- 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;
>  
>

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

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:[e-mail]] De la part de Thomas Faber
Envoyé : mardi 29 décembre 2015 13:20
À : ros-dev(a)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(a)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/dev
>  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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[Ged Murphy]

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:[e-mail]] On Behalf Of Hermès
BÉLUSCA - MAÏTO
Sent: 29 December 2015 16:08
To: 'ReactOS Development List' <ros-dev(a)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:[e-mail]] De la part de Thomas Faber
Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev(a)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(a)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/dev
>  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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

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

[Thomas Faber]

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:[e-mail]] On Behalf Of Hermès
>  BÉLUSCA - MAÏTO
>  Sent: 29 December 2015 16:08
>  To: 'ReactOS Development List' <ros-dev(a)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:[e-mail]] De la part de Thomas Faber
>  Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev(a)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(a)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/dev
> >  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;
> >
> >

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

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&…
(see iomgr/file.c and volume.c) .

-----Message d'origine-----
De : Ros-dev [mailto:[e-mail]] 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:[e-mail]] On Behalf Of Hermès 
>  BÉLUSCA - MAÏTO
>  Sent: 29 December 2015 16:08
>  To: 'ReactOS Development List' <ros-dev(a)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:[e-mail]] De la part de Thomas 
>  Faber Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev(a)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(a)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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[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:[e-mail]] 
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&…
(see iomgr/file.c and volume.c) .

-----Message d'origine-----
De : Ros-dev [mailto:[e-mail]] 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:[e-mail]] On Behalf Of Hermès 
>  BÉLUSCA - MAÏTO
>  Sent: 29 December 2015 16:08
>  To: 'ReactOS Development List' <ros-dev(a)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:[e-mail]] De la part de Thomas 
>  Faber Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev(a)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(a)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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[Ged Murphy]

Yeah good point about the SMP variant Thomas, I hadn't taken that into consideration.
When I get a free minute, I'll update the code (and various other sections that access
attached devices) to use the spinlock

-----Original Message-----
From: Ros-dev [mailto:[e-mail]] On Behalf Of Hermès BÉLUSCA - MAÏTO
Sent: 30 December 2015 10:21
To: 'ReactOS Development List' <ros-dev(a)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

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:[e-mail]] 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&…
(see iomgr/file.c and volume.c) .

-----Message d'origine-----
De : Ros-dev [mailto:[e-mail]] 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:[e-mail]] On Behalf Of Hermès 
>  BÉLUSCA - MAÏTO
>  Sent: 29 December 2015 16:08
>  To: 'ReactOS Development List' <ros-dev(a)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:[e-mail]] De la part de Thomas 
>  Faber Envoyé : mardi 29 décembre 2015 13:20 À : ros-dev(a)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(a)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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

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