The exclusive TransitionLock hold is required with all state changes
that:
- Switch adapter TUN_STATE_RUNNING -> TUN_STATE_PAUSING
- Switch media status to MediaConnectStateDisconnected
- Switch adapter to low-power state NdisDeviceStateD1-n
Because, those transitions require NBL queue to clear and stay empty.
The parts of the code that fill the NBL queue get a shared lock
before state check and release it after they are done filling the queue.
This prevents intermediate state changes while filling, and the queue
clearing parts of the code to wait for the filling parts to prevent a
race.
Starting up adapter (TUN_STATE_RESTARTING -> TUN_STATE_RUNNING,
MediaConnectStateDisconnected -> MediaConnectStateConnected, or
NdisDeviceStateDx -> NdisDeviceStateD0) does not require emptying NBL
queue and it does not need TransitionLock held.
This commit also moves TunQueueClear() out of exclusive TransitionLock
hold, because it is not necessary.
Signed-off-by: Simon Rozman <simon@rozman.si>
At the time of the NdisMSendNetBufferListsComplete() call, we're always
at Dispatch IRQL, because of ctx->TransitionLock being held.
Signed-off-by: Simon Rozman <simon@rozman.si>
Rumor has it, NT is one of the few kernels that actually has RCU
(alongside Linux and maybe some IBM things). So maybe if we ever learn
how to use NT's RCU functionality, we'll be able to replace this with
that.
TunCompleteRequest() no longer sets Information field in IRP and allows
to specify custom priority boost. This makes it suitable replacement for
all "set status; complete request; release remove lock"-tuples
throughout the code.
Functional changes in this patch:
- We no longer reset Information field to 0 for canceled IRPs. In other
words: ReadFile() of a canceled IRP will get the number of bytes read
before request was canceled in the lpNumberOfBytesRead, instead of
always 0.
- After write is complete, we boost user thread priority by +2
(IO_NETWORK_INCREMENT).
Signed-off-by: Simon Rozman <simon@rozman.si>
The NDIS reason why TunCheckForPause() reported the adapter unavailable,
might not make much sense to a client. In case adapter is paused or in
low power state, the STATUS_CANCELLED (ERROR_OPERATION_ABORTED) is
returned.
Exchange buffer size overflow - total TUN_EXCH_MAX_IP_PACKET_SIZE or
individual packet - rejects entire exchange buffer now.
Exchange buffers containing non-IPv4 or non-IPv6 packets are now
rejected as a whole.
Allocation errors while preparing NBLs from the exchange buffer are now
considered fatal.
Ensure write buffer has at least sizeof(TUN_PACKET) left, or reject
entire exchange buffer.
Signed-off-by: Simon Rozman <simon@rozman.si>
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>
No need for breaking every XML tag with attributes into lines - besides,
we're imposing 2-space indentation on .proj files making lines even
shorter and indentation combined with excessive line breaking harder to
follow visually.
Signed-off-by: Simon Rozman <simon@rozman.si>
This is temporary until we figure out how to use WiX's Difx to install
driver without leaving staged driver files behind.
Signed-off-by: Simon Rozman <simon@rozman.si>
This allows common version and other configuration in wintun.props.
- MSM packaging migrated to WiX MSBuild project and extended to support
digital signing
- Building supports single Platform|Configuration per run - again :(
- wintun.vcxproj cleanup
Signed-off-by: Simon Rozman <simon@rozman.si>
Revision (third) and build (fourth) fields were also dropped:
- NDIS_MINIPORT_DRIVER_CHARACTERISTICS supports only a.b
- MSI checks only a.b.c
- GPO compares only a.b when checking if the new MSI is an update
Signed-off-by: Simon Rozman <simon@rozman.si>
