NDIS 6.30 upgrade

[jkunkee]

Using MSDN guidance, this is a naïve upgrade of the driver to NDIS 6.30. This is required for compatibility with Windows 10 on ARM.

Since 6.20 is required for compatibility Windows 7, 6.30 support is conditional at runtime.

[djdv]

Chiming in here as well. From master, I apply https://github.com/OpenVPN/tap-windows6/pull/53 and then this. Building the driver and the installer have no errors, however, I'm experiencing issues with the driver not starting. It shows up in device manager and its event log shows this

Device not started (tap0901)
Device ROOT\NET\0000 had a problem starting.

Driver Name: oem13.inf
Class Guid: {4D36E972-E325-11CE-BFC1-08002BE10318}
Service: tap0901
Lower Filters: 
Upper Filters: 
Problem: 0x25
Problem Status: 0xC0000001

With https://github.com/OpenVPN/tap-windows6/pull/53 alone, I have no issues building the driver+installer, and the driver starts successfully after installing from that build.

Not sure if this is helpful information or not.

[jkunkee]

Which Windows version did you try it on? (winver is more than enough info for me)

[djdv]

1803 (17134.48) For both build+run

[jkunkee]

I'll have to see what the debugger shows me. I suspect it's related to the NDIS version changes because just #53 now builds and runs for me.

[jkunkee]

Current patch set fails on both Win10 and Wn7 with Code 37 (0x25). Fix is in progress.

[jkunkee]

Turns out I got a bit field shift wrong in the date calculation. This has been fixed and passes my smoke test on x64 Win10 and Win7.

[djdv]

Built, installed, and initialized on my machine (same env as I mentioned above). 👍

[cron2]

Hi,

On Tue, May 08, 2018 at 05:04:15PM -0700, Jon Kunkee wrote:

Using MSDN guidance, this is a naïve upgrade of the driver to NDIS 6.30. This is required for compatibility with Windows 10 on ARM.

Since 6.20 is required for compatibility Windows 7, 6.30 support is conditional at runtime. You can view, comment on, or merge this pull request online at:

https://github.com/OpenVPN/tap-windows6/pull/55

Being a github noob... is there an easy way to see what this would look like, rebased on current master?

(I was reading through the .patch URL given, and all the "pmCapabilities" and "NdisAllocateRWLock()" stuff is already in 6.20 / current master, so figuring out what is new for 6.30 is a bit non-obvious)

thanks,

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

[jkunkee]

Odd...I had hoped that as the earlier PRs were merged the superfluous content here would be trimmed. I'll have a look. Apologies for the mess.

[jkunkee]

@cron2, I have rebased this branch to isolate its payload for the PR. I'll have to do it again for the next two PRs.

[jkunkee]

The RWLock commit is also available in its own standalone PR: #59

[jkunkee]

Simplified history by squashing commits and leaving RWLock fix out

[jkunkee]

For future readers:

One of the key selling points for Windows 10 on ARM hardware is extreme battery life. Since NDIS 6.30 introduces some power management features that enable this, NDIS 6.20 and lower are not supported on the platform.

[jkunkee]

Squashed more commits, fixed incorrect NDIS_SIZEOF_MINIPORT usage, correctly split work between commits (so they should all build individually), adjust commit messages to reflect changes

[cron2]

Hi,

On Tue, May 08, 2018 at 05:04:15PM -0700, Jon Kunkee wrote:

Using MSDN guidance, this is a naïve upgrade of the driver to NDIS 6.30. This is required for compatibility with Windows 10 on ARM.

Since 6.20 is required for compatibility Windows 7, 6.30 support is conditional at runtime. You can view, comment on, or merge this pull request online at:

Looking through the changes, and the documentation in here

https://docs.microsoft.com/en-us/windows-hardware/drivers/network/summary-of-changes-required-to-port-a-miniport-driver-to-ndis-6-30

and there is one thing that we might want to double check

"In NDIS 6.30, NDIS can call MiniportInitializeEx twice in parallel if there are two adapters plugged into the system at the same time or during system startup. Be sure to test your miniport driver under this "parallel startup" condition"

so, if we have two or more TAP adapters with the same drivers, this sounds like it would hit adapter.c:CreateAdapter() - the only reference I could find to "InitializeEx" is this one

    miniportCharacteristics.InitializeHandlerEx = AdapterCreate;

so is that "MiniportInitializeEx" or is this something we do not use at all, so we're safe?

Besides this (which may just be me being confused) this looks all according to specs - if the system is NDIS6.30, we use the right data structure (which, I assume, works because the _REVISION_2 only adds members at the end and all the rest stays the same), and set the NDIS_MINIPORT_ATTRIBUTES_NO_PAUSE_ON_SUSPEND thing.

So, a careful "ACK-if"...

@selvanair, do you spot anything?

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]

I had only one concern which I wanted to test but couldn't find time:

How would NDIS_MINIPORT_ATTRIBUTES_NO_PAUSE_ON_SUSPEND flag interact with the userspace? iiuc, this would skip the calls to AdapterPause() and AdpaterRestart(). Currently the adapter "halts" on suspend, triggering a tun read/write error in openvpn which we handle by a issuing a delayed restart of the tunnel. Before we started doing that there used to be many reports about improper recovery from suspend/resume (or sleep). So, with this flag on, will the read/write error still trigger causing openvpn to restart?

While a virtual adapter has no reason to "halt" on lowering power or even hibernate, and a smoother transition during suspend resume would be good, there is one catch. Any reasonable amount of suspend time will anyway make the server to conclude the client is gone (assuming udp). In that case the client on wake up may take a long time (internal ping timeout = a couple of minutes) to realize that it has to restart. If so, this would be worse than the current situation where the tunnel re-establishes in a couple of seconds after wakeup.

@cron2 : AdapterCreate is indeed the MiniportInitializeEx() in the docs and to me it looks safe to call it in parallel. Still, running some tests with multiple adapters may be useful although its not easy to rule out race conditions by tests.

[jkunkee]

I agree with @selvanair on parallel startup.

As for NDIS_MINIPORT_ATTRIBUTES_NO_PAUSE_ON_SUSPEND, this was naïve of me. I agree with your reasoning and will pull it out.

[jkunkee]

commit message fixed too :)

[cron2]

Hi,

On Mon, Jun 11, 2018 at 08:53:13PM +0000, Selva Nair wrote:

@cron2 : AdapterCreate is indeed the MiniportInitializeEx() in the docs and to me it looks safe to call it in parallel. Still, running some tests with multiple adapters may be useful although its not easy to rule out race conditions by tests.

Thanks for checking.

I was a bit confused by the allocation of memory in AdapterCreate() -> tapAdapterContextAllocate() (which is not locked etc.) - but after re-reading this is "per instance" memory so allocating twice is perfectly in order - and does not need locking, as it won't touch "the other instance" (or "hardware" :-) ).

Access to adapter->Locked.AdapterState is nicely locked, but that's not vs. multiple CreateAdapter() calls but vs. other threads accessing this instance's AdapterState.

So anyway, just ignore me :-)

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

[cron2]

@jkunkee could you rebase this so it can be one-click merged? The "RunningWindows8OrGreater" change to tap.h seems to cause a conflict here (and I'm a bit wary with all the various open PRs to "just fix it"). thanks.

[cron2]

Github was confusing me... now it just merged cleanly.