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

[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