Commentary from Jason:
Problem statement:
We call IoCompleteRequest(Irp) immediately after
NdisMIndicateReceiveNetBufferLists, which frees Irp->MdlAddress.
Since we've just given the same memory to
NdisMIndicateReceiveNetBufferLists (in a different MDL), we wind up
freeing the memory before NDIS finishes processing them.
Fix possibility 1:
Move IoCompleteRequest(Irp) to TunReturnNetBufferLists. This requires
reference counting how many NBLs are currently in flight that are
using an IRP. When that drops to zero, we can call IoCompleteRequest
(Irp).
Problem:
This means we have to block future wireguard-go Writes until *all*
NBLs have completed processing in the networking stack. Is that safe
to do? Will that introduce latency? Can userspace processes sabotage
it by refusing to read from a TCP socket buffer? We don't know enough
about how NdisMIndicateReceiveNetBufferLists works to assess its
characteristics here.
Fix possibility 2:
Use NDIS_RECEIVE_FLAGS_RESOURCES, so that
NdisMIndicateReceiveNetBufferLists makes a copy, and then we'll simply
free everything immediately after. This is slow, and it could
potentially lead to wireguard-go making the kernel allocate lots of
memory in the case that NdisAllocateNetBufferAndNetBufferList doesn't
ratelimit its creation in the same way Linux's skb_alloc does.
However, it does make the lifetime of Irps shorter, which is easier to
analyze, and it might lead to better latency, since we don't need to
wait until userspace sends its next packets, so long as Ndis'
ingestion queue doesn't become too large.
This commit switches from (2) to (1).
Signed-off-by: Simon Rozman <simon@rozman.si>
When adapter is in disconnected state, NDIS does not send it any NBLs.
After transition to disconnected state it should return all pending NBLs
back to NDIS, otherwise a deadlock occurs on pause attempt later.
Likewise when the adapter is in low-power state.
Signed-off-by: Simon Rozman <simon@rozman.si>
MINIPORT_OID_REQUEST handler should return:
- NDIS_STATUS_NOT_SUPPORTED => NDIS_STATUS_INVALID_OID if the OID
request was not recognized.
- NDIS_STATUS_INVALID_OID => NDIS_STATUS_NOT_SUPPORTED if particular OID
is not supported.
- NDIS_STATUS_INVALID_LENGTH => NDIS_STATUS_BUFFER_TOO_SHORT if output
buffer is too short.
Signed-off-by: Simon Rozman <simon@rozman.si>
These constants must be defined in the project file in order for Static
Driver Verifier to work correctly. Otherwise, SDV returns error MSB3073:
The command "staticdv /check:*" exited with code -1.
Since the ClCompile/PreprocessorDefinitions management became
cumbersome, all platform and configuration independent MSVC settings
were rearranged in a single <ItemDefinitionGroup>, while platform
independent but configuration dependent were rearranged to a conditional
<ItemDefinitionGroup>(s).
Signed-off-by: Simon Rozman <simon@rozman.si>
This allows Windows to unload the driver from memory when the last
adapter is halted. Hence driver can be updated without a reboot.
Unfortunately, a client refusing to close device pipe handle can block
adapter halting indefinitely. So, we now have a new challenge to
address.
Signed-off-by: Simon Rozman <simon@rozman.si>
This always sets the reference counter to predictable state on resume
and eliminates the need to bump it on pausing.
Signed-off-by: Simon Rozman <simon@rozman.si>
This unblocks waiting clients and prevents new handles to be opened on
device pipe while allowing graceful cleanup.
Signed-off-by: Simon Rozman <simon@rozman.si>
When transmitting, the adapter can actually take TUN_QUEUE_MAX_NBLS
packets at a time.
When receiving, the adapter gets up to TUN_EXCH_MAX_PACKETS packets at
a time.
Signed-off-by: Simon Rozman <simon@rozman.si>
When driver cannot access IRP's MDL it shouldn't grand calling user
thread any priority boost. The IO_NETWORK_INCREMENT was overlooked on
copy&paste.
Signed-off-by: Simon Rozman <simon@rozman.si>
Pausing an adapter requires it to stop processing network traffic. We
may still process I/O of the adapter device pipe.
By canceling all pending IRPs and refusing to serve any new IRPs on
adapter pause, we forcibly cut the client off to release resources and
allow graceful driver unload. This enabled a hot driver update without a
restart.
However, using a modified Wintun driver for NDISTEST utility the client
didn't behave the same way, keeping handles open when NDISTEST paused
Wintun adapter. The pause reset ctx->Device.RefCount to 0 which caused
negative reference count when client finally decided to close the Wintun
adapter device pipe handle.
Rather than introducing a complex fix to the adapter device pipe
reference counting, this commit simplifies it by allowing IRPs even when
adapter is paused. IRP_MJ_WRITE is an exception: client cannot write to
the adapter when it's paused.
This allows the client to stay connected, reduces ActiveTransactionCount
to NBL counting only, and cancels pending IRPs much later in
TunHaltEx().
Since there is still I/O going on after TunHaltEx() returns now, the
driver is not unloaded from memory. This prevents driver updates without
a reboot. How can one gracefully terminate I/O in TunHaltEx() remains a
subject of further study.
Signed-off-by: Simon Rozman <simon@rozman.si>
This commit addresses issues reported by NDISTEST/1c_64bitoids.
It also revises the OID request writing, as adaptive 32/64-bit responses
required by OID_GEN_XMIT_OK and OID_GEN_RCV_OK would grow the current
TunOidQuery() design quite complex and introduce duplicated code.
Signed-off-by: Simon Rozman <simon@rozman.si>
NDIS >=6.30 allows bringing adapter to low-power state without pausing
it. As Wintun's pause cancels all pending IRP and we want to keep
disturbing client to a minimum, OID_PNP_SET_POWER cancels pending NBLs
and wait for active NBLs to finish only.
Signed-off-by: Simon Rozman <simon@rozman.si>
All parameters to TunOidSet/TunOidQuery() are contained inside a
NDIS_OID_REQUEST struct. But, the main rationale behind this commit is
that OID_PNP_SET_POWER set request will require pointer to original
NDIS_OID_REQUEST struct for NdisMOidRequestComplete() call, should we
want to implement pending PnP power-down.
Signed-off-by: Simon Rozman <simon@rozman.si>
Outgoing statistics-keeping was accidentally dropped in
ed93692ca0049123979e0497aa3c4a4698f844cd. This commit reintroduces it.
Signed-off-by: Simon Rozman <simon@rozman.si>
Screen is a valuable real-estate on small displays or when working in
less than maximized or full-screen Visual Studio IDE.
This commit replaces tabulators used for horizontal alignment with
spaces, and stops imposing 8-space tabulators on everyone.
Signed-off-by: Simon Rozman <simon@rozman.si>
This also introduces a proper queueing and refcounting system, which
should increase performance and allow for IRP cancelation.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Simon Rozman <simon@rozman.si>
If device connection is not terminated on NDIS pause, the driver doesn't
unload when adapter is disabled. This also prevents driver updates
without reboot.
Signed-off-by: Simon Rozman <simon@rozman.si>
The ActiveNBLCount must be bumped _before_ adapter state is checked.
Otherwise, there is a window of code that may allow adapter transition
from pausing to paused state, followed by a
NdisMIndicateReceiveNetBufferLists() call.
Signed-off-by: Simon Rozman <simon@rozman.si>
