This is actually very much wrong. In fact, it's bound to create all
sorts of nasty issues. But without it, we can't use the reference
function to check the validity of a potentially invalid handle while the
verifier is enabled.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
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>
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>
