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, [e-mail] 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
À : [e-mail]
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, [e-mail] 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
[e-mail]
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' [e-mail]
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 À : [e-mail] 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, [e-mail] 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
[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev

_______________________________________________
Ros-dev mailing list
[e-mail]
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' [e-mail]
> 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 À : [e-mail] 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, [e-mail] 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&a... (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' [e-mail]
> 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 À : [e-mail] 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, [e-mail] 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
[e-mail]
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&a... (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' [e-mail]
> 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 À : [e-mail] 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, [e-mail] 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
[e-mail]
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' [e-mail]
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&a... (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' [e-mail]
> 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 À : [e-mail] 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, [e-mail] 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
[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev

_______________________________________________
Ros-dev mailing list
[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev