[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Aric Stewart]

Thanks for the review!

On 1/22/16 10:02 PM, Sebastian Lackner wrote:
> On 22.01.2016 19:15, Aric Stewart wrote:
>> Code is mostly moved from winedevice.exe
>> winedevice.exe now uses these functions
>> v3: No longer use advapi32 Registry functions
>> v4: Fix FALSE return code (thanks to Huw Davies for spotting this one)
>>      Remove WINE_ from debuging macros
>>
>> Signed-off-by: Aric Stewart <aric at codeweavers.com>
>> ---
>>   dlls/ntoskrnl.exe/ntoskrnl.c        | 306 ++++++++++++++++++++++++++++++++++++
>>   dlls/ntoskrnl.exe/ntoskrnl.exe.spec |   4 +-
>>   programs/winedevice/device.c        | 243 +---------------------------
>>   3 files changed, 313 insertions(+), 240 deletions(-)
>>
> 
> I don't think moving the loader code from winedevice to ntoskrnl.exe is a good idea,
> at least not without further changes, because:
> 
> * We should not initialize the same driver twice, the second initialization could
>    fail because of namespace collisions.
> 
> * Drivers should be properly uninitialized, no matter how they were loaded.
> 
> * In theory, all drivers should be loaded into the same address space. This means,
>    winedevice and the wineserver has to keep track of them, and its not sufficient to
>    maintain a list in ntoskrnl.exe.
> 
> Besides those design issues, I've marked a couple of other issues in the patch below.


I am going to let you and Alexandre continue to discuss this.  In theory you are correct, if we had proper kernel ring address space and such that would be true, but we don't. So we have to try to make do with what we can. What this does do is it lets my plug and play manager be able to load all its drivers into its own process space so that they can communicate. It opens the door to allowing things like that.

So if you have 2(+) drivers that need to be in the same process space then we can look at things like winedevice from being a service that loads each driver into a new process to being a single process that load each driver requested. But loading things like that into the same process is very fragile since if any driver crashes it can take down all the drivers in that process.


> 
>>
>> 0001-ntoskrnl.exe-Implement-ZwLoadDriver-and-ZwUnloadDriver.txt
>>
>>
>> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
>> index 04f0198..032ab21 100644
>
.
.
.
> It wouldn't hurt to add locking for such a global list.

Agreed, Added.  If we move this to ntdll then we can use the existing loader lock.

.
.
.
>> +/***********************************************************************
>> + *           ZwLoadDriver (NTOSKRNL.EXE.@)
>> + */
>> +NTSTATUS WINAPI ZwLoadDriver(const UNICODE_STRING *DriverServiceName)
> 
> Both the Nt* and Zw* function should do the same, so I would suggest to implement
> Nt* instead and forward Zw*, like its done in ntdll.
> 

I would love to head Alexandre chime in on this. I could go either way. In our private discussions he lead me toward ZwLoadDriver but maybe I misunderstood and I can move this to NtLoadDriver if that is what is better. 

Your other comments are also very helpful and I have made those fixes.

I will not submit a v5 until we figure out if this should remain in ntoskrnl or move to ntdll but then I will push a v5.

Thanks again for the review!

-aric