Re: [PATCH 1/1] in order to prevent buffer overrun (which was observed while sending multiple high throughput UDP streams from different threads) I move the driver spinlock to protect Ring buffer Head.
Oded Katz <[email protected]> Fri, 27 Feb 2026 13:28:05 -0800
| Newsgroups | com.zx2c4.lists.wireguard |
|---|---|
| Message-ID | <CAApnxP3H7LjxDBfFoPMsjZ=fb1mosOaemLW-c5xgdhuzWQvzfA@mail.gmail.com> |
Hi Simon, We have found another issue with the driver code. some race condition between "alertable" flag between driver and user-side. causing driver to stall until new packet wakes it up. We have seen some cases where it can be for periods of 5 sec. we already made the fix, and I sent 2 approaches for that fix. please let us know if you are planning to solve it in your repo? regards and thanks, Oded On Fri, Feb 20, 2026 at 8:55=E2=80=AFAM Oded Katz <[email protected]> wro= te: > > Hi Simon, > > Thanks for the update. > FYI: the ReadULongAcquire() and WriteULongRelease() themself are good > enough for getting and updating an atomic operation. > however, after on thread updates the head, the second thread can't > stay with the same atomic read value of the Head. > - so assuming 2 thread read the Head on the same time so the 2 threads > has same head. > - now 1st thread takes the spin lock and updates the head > - 2nd thread is pending the job until the spinlock is released and > then it doing the job, but with the non-updated Head value > - so it will overrun the previous thread data and mess the ringbuffe= r > > > again thanks for providing the next release > Regards, > Oded > > On Thu, Feb 19, 2026 at 11:28=E2=80=AFPM Simon Rozman <simon.rozman@amebi= s.si> wrote: > > > > Hi, Oded! > > > > First and foremost, thank you very much for taking time to look into > > this and troubleshoot it. > > > > It was believed, that ReadULongAcquire() and WriteULongRelease() alone > > provide atomic manipulation with Ring->Head on all modern platforms. > > Hence, these calls were made outside the spinlock, to squeeze an extra > > micrometer of performance. > > > > I could not directly apply your patch to the wintun repo, since it does > > not follow our code style, commit message conventions and is not > > Signed-off-by you. > > > > However, it would have been a terrible waste of your research if your > > contribution wouldn't get into wintun, so I reworked your PR and applie= d > > it here: > > https://git.zx2c4.com/wintun/commit/?id=3D607c181ea9fa0036d23598038e601= 9ad54db5ce4 > > > > Please, stay tuned for an official WHQL-signed release. > > > > Lep pozdrav | Best regards, > > Simon Rozman > > Amebis, d. o. o., Kamnik > > > > On 19. 2. 2026 20.32, odedkatz wrote: > > > I observed that the Ring->Head was taken and manipulated later o= n with just a `ReadULongAcquire` which isn't OK when 2 are trying to manipu= late it later on based on the same received value. > > > --- > > > driver/wintun.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > diff --git a/driver/wintun.c b/driver/wintun.c > > > index d1f3b9f..65cd97e 100644 > > > --- a/driver/wintun.c > > > +++ b/driver/wintun.c > > > @@ -284,13 +284,14 @@ TunSendNetBufferLists( > > > TUN_RING *Ring =3D Ctx->Device.Send.Ring; > > > ULONG RingCapacity =3D Ctx->Device.Send.Capacity; > > > > > > + KLOCK_QUEUE_HANDLE LockHandle; > > > + KeAcquireInStackQueuedSpinLock(&Ctx->Device.Send.Lock, &LockHand= le); > > > /* Allocate space for packets in the ring. */ > > > ULONG RingHead =3D ReadULongAcquire(&Ring->Head); > > > - if (Status =3D NDIS_STATUS_ADAPTER_NOT_READY, RingHead >=3D Ring= Capacity) > > > + if (Status =3D NDIS_STATUS_ADAPTER_NOT_READY, RingHead >=3D Ring= Capacity) { > > > + KeReleaseInStackQueuedSpinLock(&LockHandle); > > > goto skipNbl; > > > - > > > - KLOCK_QUEUE_HANDLE LockHandle; > > > - KeAcquireInStackQueuedSpinLock(&Ctx->Device.Send.Lock, &LockHand= le); > > > + } > > > > > > ULONG RingTail =3D Ctx->Device.Send.RingTail; > > > ASSERT(RingTail < RingCapacity); > > > @@ -419,8 +420,8 @@ TunReturnNetBufferLists(NDIS_HANDLE MiniportAdapt= erContext, PNET_BUFFER_LIST Net > > > Ctx->Device.Receive.ActiveNbls.Head =3D NET_BUFFER_LIST= _NEXT_NBL_EX(CompletedNbl); > > > if (!Ctx->Device.Receive.ActiveNbls.Head) > > > KeSetEvent(&Ctx->Device.Receive.ActiveNbls.Empty, I= O_NO_INCREMENT, FALSE); > > > - KeReleaseInStackQueuedSpinLock(&LockHandle); > > > WriteULongRelease(&Ring->Head, TunNblGetOffset(Complete= dNbl)); > > > + KeReleaseInStackQueuedSpinLock(&LockHandle); > > > const MDL *TargetMdl =3D Ctx->Device.Receive.Mdl; > > > for (MDL *Mdl =3D NET_BUFFER_FIRST_MDL(NET_BUFFER_LIST_= FIRST_NB(CompletedNbl)); Mdl; Mdl =3D Mdl->Next) > > > { > >