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]> Wed, 11 Mar 2026 16:35:25 -0700
| Newsgroups | com.zx2c4.lists.wireguard |
|---|---|
| Message-ID | <CAApnxP3-0gSkh7W0Y8OCHVQ05phWfG9hY6jTnjR0EoUVuxgvPw@mail.gmail.com> |
Hi Simon,
I still haven't heard from you guys, neither saw a ticket in the mailing li=
st.
is there any update ?
regards,
Oded
On Thu, Feb 19, 2026 at 11:28=E2=80=AFPM Simon Rozman <simon.rozman@amebis.=
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 applied
> it here:
> https://git.zx2c4.com/wintun/commit/?id=3D607c181ea9fa0036d23598038e6019a=
d54db5ce4
>
> 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 on =
with just a `ReadULongAcquire` which isn't OK when 2 are trying to manipula=
te 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, &LockHandle=
);
> > /* Allocate space for packets in the ring. */
> > ULONG RingHead =3D ReadULongAcquire(&Ring->Head);
> > - if (Status =3D NDIS_STATUS_ADAPTER_NOT_READY, RingHead >=3D RingCa=
pacity)
> > + if (Status =3D NDIS_STATUS_ADAPTER_NOT_READY, RingHead >=3D RingCa=
pacity) {
> > + KeReleaseInStackQueuedSpinLock(&LockHandle);
> > goto skipNbl;
> > -
> > - KLOCK_QUEUE_HANDLE LockHandle;
> > - KeAcquireInStackQueuedSpinLock(&Ctx->Device.Send.Lock, &LockHandle=
);
> > + }
> >
> > ULONG RingTail =3D Ctx->Device.Send.RingTail;
> > ASSERT(RingTail < RingCapacity);
> > @@ -419,8 +420,8 @@ TunReturnNetBufferLists(NDIS_HANDLE MiniportAdapter=
Context, PNET_BUFFER_LIST Net
> > Ctx->Device.Receive.ActiveNbls.Head =3D NET_BUFFER_LIST_N=
EXT_NBL_EX(CompletedNbl);
> > if (!Ctx->Device.Receive.ActiveNbls.Head)
> > KeSetEvent(&Ctx->Device.Receive.ActiveNbls.Empty, IO_=
NO_INCREMENT, FALSE);
> > - KeReleaseInStackQueuedSpinLock(&LockHandle);
> > WriteULongRelease(&Ring->Head, TunNblGetOffset(CompletedN=
bl));
> > + KeReleaseInStackQueuedSpinLock(&LockHandle);
> > const MDL *TargetMdl =3D Ctx->Device.Receive.Mdl;
> > for (MDL *Mdl =3D NET_BUFFER_FIRST_MDL(NET_BUFFER_LIST_FI=
RST_NB(CompletedNbl)); Mdl; Mdl =3D Mdl->Next)
> > {
>