OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
788 stars 237 forks source link

Move RWLock alloc to use NdisHandle #59

Closed jkunkee closed 6 years ago

jkunkee commented 6 years ago

This is required for Windows 7 per the documentation for NdisAllocateRWLock.

cron2 commented 6 years ago

Hi,

On Wed, May 30, 2018 at 06:28:42PM -0700, Selva Nair wrote:

NdisMRegisterMiniportDriver() will call AdapterCreate() callback (the InitializeHandlerEx callback) which will call tapAdapterContextAddToGlobalList() which needs the lock.

Can we create the lock in AdapterCreate()?

(Well spotted, btw, I thought this would be the easy and logical way to adapt things...)

gert

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany gert@greenie.muc.de

selvanair commented 6 years ago

My remark is based on the following in the docs for NdisMRegisterMiniportDriver() here: https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndismregisterminiportdriver

After a driver calls NdisMRegisterMiniportDriver, the driver should be prepared to be called back at the MiniportInitializeEx function that is specified in the MiniportDriverCharacteristics parameter.

But another source (https://docs.microsoft.com/en-us/windows-hardware/drivers/network/initializing-a-miniport-driver) has this:

Drivers that call NdisMRegisterMiniportDriver must be prepared for NDIS to call their MiniportInitializeEx functions any time after DriverEntry returns. (emphasis mine).

If the latter is the right description we are safe. So if @jkunkee can confirm that call never happens before Driver entry returns, we are ok as is.

jkunkee commented 6 years ago

This sample has lock initialization after the call to NdisMRegisterMiniportDriver, during DriverEntry, so that's good. I can still ask the NDIS team for clarification of the docs.

selvanair commented 6 years ago

Yeah, I've seen that sample. But the docs seem to imply otherwise --- it would be safe to get some clarification. Thanks.

jkunkee commented 6 years ago

Per the NDIS team's response, this is the correct version:

Drivers that call NdisMRegisterMiniportDriver must be prepared for NDIS to call their MiniportInitializeEx functions any time after DriverEntry returns.

selvanair commented 6 years ago

That settles it, I guess.

One more thing: if the NdisAllocateRWLock fails, NdisFreeRWLock() will get called with A NULL (in unload). This may be ok (?), but better check for NULL and also set the lock pointer to NULL after freeing.

Looks good otherwise.

jkunkee commented 6 years ago

Done and done.

mattock commented 6 years ago

@jkunkee would it be possible to harmonize the two documents above? Or are those outside the jurisdiction of the NDIS team? Just thinking of the future poor souls who get confused about this :smile:.

cron2 commented 6 years ago

I take Selva's last comment as an ACK-if, and the change has been made :-) - and my understanding of code and clarified documentation also says "this is good, it should go in". So, done.