usb: let only one party reply for a seqnum

[potpiemuncher]

USBIP_CMD_UNLINK and the async IN completion goroutines can currently both write a reply for the same seqnum, so a client can receive RET_UNLINK(-ECONNRESET) and RET_SUBMIT for one request.

Found by code reading against main @ 308e9b2 while auditing lifecycle invariants for a downstream project. Not reproduced — the window is narrow and I have no failing trace. Reporting and fixing it because of what the consequence looks like on the client side, and because the fix is small. (Issues are disabled on this repo, so a PR seemed the right channel — happy to reshape it however you prefer.)

The asymmetry

The UNLINK handler already claims ownership properly — it only answers -ECONNRESET if it was the one to remove the entry, otherwise status = 0 meaning "already completed":

pendingMu.Lock()
cancel, found := pending[unlinkSeq]
if found {
    delete(pending, unlinkSeq)
}
pendingMu.Unlock()

Both async IN completion paths delete unconditionally, discard the result, and write regardless — server.go:967-971 (ISO-IN) and the same shape at server.go:1013-1018 (generic IN). writeRet applies no ownership test of its own; it serialises on writeMu and writes a RET_SUBMIT for whatever seqnum it is handed.

The interleaving

The ISO-IN goroutine checks urbCtx at 933, 949 and 955, then takes pendingMu at 967. If an UNLINK arrives in the gap between that last context check and the lock:

1. UNLINK finds the entry, deletes it, cancels, writes RET_UNLINK(-ECONNRESET).
2. The completion deletes nothing, notices nothing, and writes RET_SUBMIT.

Cancelling the context doesn't prevent it, because there's no further urbCtx check after 955.

Why it seemed worth fixing unreproduced

On the Windows client this lands in usbip2_ude, and a RET_SUBMIT for a request the driver has already unlinked is a response for a request it no longer owns — the same shape as the defect class behind vadimgrn/usbip-win2#181 (root-caused since to a usbip2_filter overrun, with separate request-lifetime work in #182). I'm not claiming this path causes those crashes — only that it produces exactly the input those paths are least robust against, and it's cheap not to.

The change

Extracted the removal into claimPending, which reports whether the caller was the one that removed the entry, and routed all three sites through it so the rule lives in one place:

func claimPending(mu *sync.Mutex, pending map[uint32]context.CancelFunc,
    seq uint32) (context.CancelFunc, bool) {
    mu.Lock()
    defer mu.Unlock()
    cancel, owned := pending[seq]
    delete(pending, seq)
    return cancel, owned
}

The UNLINK handler keeps its exact semantics — deleting an absent key is a no-op, so folding its conditional delete into the helper changes nothing. The two completion paths now return silently when they lose the race, with a debug line.

Credit where it's due

The stranding half is clearly deliberate — server.go:929 carries // Cancellation must never strand a later URB behind this one., and the early-exit paths at 935, 951 and 958 correctly delete without writing. Only the two success paths write without checking, which is why this reads as an oversight rather than a missing concept.

Verification

• go build ./..., go vet, full go test ./... — all pass; gofmt clean.
• New pending_test.go: four tests covering the claim, including 64 goroutines contending for one seqnum across 200 rounds under -race, where exactly one must win.
• Negative control: with the pre-fix semantics restored, all four fail — the contention test reporting 64 winners, want exactly 1. Restored and re-verified green.

[hbashton]

I have looked into this, and determined that it was due to a process trying to take over the USB stack, the process on my machine was Citrix Workspace. It is a work application, and I've added safeguards into my app to prevent this, but it is definitely still an issue upstream of USBIP