From 5da1096a91bc18c4caa474ea6e76d4b70d446fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Wed, 10 Jun 2026 09:22:41 +0800 Subject: [PATCH] usbip: use a dedicated overlapped event per VBoxUSB ioctl Device and Monitor shared one manual-reset event across all in-flight IOCTLs, while the session layer runs one goroutine per endpoint: the first completion released every GetOverlappedResult waiter with the first operation's byte count, returning before the driver finished writing the other buffers. Each overlappedIoctl call now owns its event, making concurrent URBs (and aborts) on one handle safe. --- common/vboxusb/device_windows.go | 47 +++++++++++++------------------ common/vboxusb/monitor_windows.go | 32 ++++----------------- 2 files changed, 25 insertions(+), 54 deletions(-) diff --git a/common/vboxusb/device_windows.go b/common/vboxusb/device_windows.go index ac90d16f9..8544f99ca 100644 --- a/common/vboxusb/device_windows.go +++ b/common/vboxusb/device_windows.go @@ -14,13 +14,12 @@ import ( "golang.org/x/sys/windows" ) -// Device holds an open handle to one VBoxUSB-claimed USB device plus a -// private event for overlapped I/O. Methods are not safe for -// concurrent use on the same Device — the session layer serializes -// per-endpoint via per-endpoint goroutines. +// Device holds an open handle to one VBoxUSB-claimed USB device. +// Methods are safe for concurrent use: the session layer runs one +// goroutine per endpoint, so URBs for different endpoints (and aborts) +// overlap on this handle, each with its own OVERLAPPED + event. type Device struct { handle windows.Handle - event windows.Handle closing sync.Once closeErr error } @@ -51,34 +50,17 @@ func OpenDevice(interfacePath string) (*Device, error) { // Skip IOCP wakeup on synchronous completion. Tolerated on Windows // 7+; ignore errors since the slow path still works. _ = windows.SetFileCompletionNotificationModes(handle, windows.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS) - event, err := windows.CreateEvent(nil, 1, 0, nil) - if err != nil { - windows.CloseHandle(handle) - return nil, E.Cause(err, "vboxusb: create event") - } - return &Device{handle: handle, event: event}, nil + return &Device{handle: handle}, nil } // Close releases the handle. Aborts any in-flight IOCTLs (they return // ERROR_OPERATION_ABORTED). Idempotent. func (d *Device) Close() error { d.closing.Do(func() { - var errs []error if d.handle != 0 { - err := windows.CloseHandle(d.handle) - if err != nil { - errs = append(errs, err) - } + d.closeErr = windows.CloseHandle(d.handle) d.handle = 0 } - if d.event != 0 { - err := windows.CloseHandle(d.event) - if err != nil { - errs = append(errs, err) - } - d.event = 0 - } - d.closeErr = E.Errors(errs...) }) return d.closeErr } @@ -268,13 +250,22 @@ func (e *URBStatusError) Error() string { } func (d *Device) ioctl(code uint32, in []byte, out []byte) (uint32, error) { - return overlappedIoctl(d.handle, code, in, out, d.event) + return overlappedIoctl(d.handle, code, in, out) } -func overlappedIoctl(handle windows.Handle, code uint32, in []byte, out []byte, event windows.Handle) (uint32, error) { +// overlappedIoctl issues one DeviceIoControl with a dedicated +// OVERLAPPED and event. Sharing one event across simultaneously +// pending IOCTLs is forbidden by the overlapped-I/O contract: the +// first completion would release every waiter with the first +// operation's results. +func overlappedIoctl(handle windows.Handle, code uint32, in []byte, out []byte) (uint32, error) { + event, err := windows.CreateEvent(nil, 1, 0, nil) + if err != nil { + return 0, err + } + defer windows.CloseHandle(event) var overlapped windows.Overlapped overlapped.HEvent = event - _ = windows.ResetEvent(event) var inPtr *byte var inLen uint32 if len(in) > 0 { @@ -288,7 +279,7 @@ func overlappedIoctl(handle windows.Handle, code uint32, in []byte, out []byte, outLen = uint32(len(out)) } var returned uint32 - err := windows.DeviceIoControl(handle, code, inPtr, inLen, outPtr, outLen, &returned, &overlapped) + err = windows.DeviceIoControl(handle, code, inPtr, inLen, outPtr, outLen, &returned, &overlapped) if err != nil && !errors.Is(err, windows.ERROR_IO_PENDING) { return 0, err } diff --git a/common/vboxusb/monitor_windows.go b/common/vboxusb/monitor_windows.go index 3b9c70695..df21787d1 100644 --- a/common/vboxusb/monitor_windows.go +++ b/common/vboxusb/monitor_windows.go @@ -14,14 +14,11 @@ import ( // Monitor is a handle to \\.\VBoxUSBMon. It is shared across the // process (one global monitor handle per sing-box server); per-device -// filters are added/removed against it. Concurrent ADD_FILTER / -// REMOVE_FILTER calls are serialized inside the driver but our handle -// reuses a single overlapped event — so Monitor methods are not safe -// for concurrent use across goroutines. The caller (the export host) -// is expected to serialize. +// filters are added/removed against it. Methods are safe for +// concurrent use: each IOCTL carries its own OVERLAPPED + event and +// the driver serializes filter mutations internally. type Monitor struct { handle windows.Handle - event windows.Handle closing sync.Once closeErr error } @@ -52,32 +49,15 @@ func OpenMonitor() (*Monitor, error) { } return nil, E.Cause(err, "vboxusb: open monitor") } - event, err := windows.CreateEvent(nil, 1, 0, nil) - if err != nil { - windows.CloseHandle(handle) - return nil, E.Cause(err, "vboxusb: create monitor event") - } - return &Monitor{handle: handle, event: event}, nil + return &Monitor{handle: handle}, nil } func (m *Monitor) Close() error { m.closing.Do(func() { - var errs []error if m.handle != 0 { - err := windows.CloseHandle(m.handle) - if err != nil { - errs = append(errs, err) - } + m.closeErr = windows.CloseHandle(m.handle) m.handle = 0 } - if m.event != 0 { - err := windows.CloseHandle(m.event) - if err != nil { - errs = append(errs, err) - } - m.event = 0 - } - m.closeErr = E.Errors(errs...) }) return m.closeErr } @@ -126,7 +106,7 @@ func (m *Monitor) RemoveFilter(id uint64) error { } func (m *Monitor) ioctl(code uint32, in []byte, out []byte) (uint32, error) { - return overlappedIoctl(m.handle, code, in, out, m.event) + return overlappedIoctl(m.handle, code, in, out) } // encodeFilter builds a 312-byte USBFILTER packed struct matching the