Add support for multi-process port sharing with CIBIR.#5798
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5798 +/- ##
==========================================
+ Coverage 84.80% 85.08% +0.27%
==========================================
Files 60 60
Lines 18731 18731
==========================================
+ Hits 15885 15937 +52
+ Misses 2846 2794 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
guhetier
left a comment
There was a problem hiding this comment.
In general, I am concerned that we keep adding incremental exceptions to the port reservation logic to solve the next issue, but without having a clear design goal.
This code will be hard to maintain and confusing for apps as there is no simple rule about what can be done with ports.
I think we need to take the time soon to come up with a clear story about when can port be shared and when they can't + document it + check we implement it.
I agree. I added a CIBIR.md. |
please see the updated XDP.md and CIBIR.md |
|
Maybe we should actually go through the rest of the standardization process for https://datatracker.ietf.org/doc/draft-banks-quic-cibir/ since you're still using it.... |
Do you have more context about it? I am interested in understanding whether we should work on standardizing CIBIR or if another solution was adopted by other deployments that we should align with. |
I know of no other solution/proposal that achieves what CIBIR does. We can chat more on Discord about the process. |
mtfriesen
left a comment
There was a problem hiding this comment.
This is looking close, aside from the refinements requested by various comments.
|
The PR description says:
But errors when setting up XDP rules seems to be silently eaten: When XDP is not backed up with OS sockets, we would silently ignore all traffic. Probably applies to QTIP too. |
Yeah, looking through the code, xdp failures in rule plumbing get silently ignored. This is a latent bug impacting QTIP (now CIBIR as well). Let's create a separate issue for this and address it in a follow up PR. This PR is starting to balloon into a very hard to review size. |
|
guhetier
left a comment
There was a problem hiding this comment.
Looks good to me, two minor nits and it is ready to go.
Fixes #5935 ## Problem `CxPlatDpRawPlumbRulesOnSocket`, `CxPlatDpRawInterfaceAddRules`, and `CxPlatDpRawInterfaceUpdateRules` all returned `void`, silently dropping failures from `XdpCreateProgram`, allocation failures, and rule overflow. When XDP is required without OS socket fallback (e.g. CIBIR + XDP server sockets as introduced in #5798), a silent failure means the socket accepts no traffic with no indication to the application. ## Fix Changed all three functions to return `QUIC_STATUS` and propagate errors up through the call chain to the caller. **`CxPlatDpRawInterfaceUpdateRules`:** Tracks the first `XdpCreateProgram` failure across all queues and returns it after all queues are attempted. **`CxPlatDpRawInterfaceAddRules`:** Returns `QUIC_STATUS_BUFFER_TOO_SMALL` on rule overflow, `QUIC_STATUS_OUT_OF_MEMORY` on alloc failure, and propagates the return value from `CxPlatDpRawInterfaceUpdateRules`. Fixed a pre-existing PortSet memory leak — the buffer is now freed when `AddRules` fails before copying the rule into `Interface->Rules`. **`CxPlatDpRawPlumbRulesOnSocket`:** Propagates failures from `CxPlatDpRawInterfaceAddRules`. On failure, both Wildcard and non-Wildcard branches now break at the first failure and perform best-effort rollback of already-configured interfaces before returning. The Wildcard branch calls `CxPlatDpRawInterfaceRemoveRules` on interfaces that were already configured. The non-Wildcard branch clears the port bit on interfaces that were already configured. **`datapath_raw.h`:** Updated declaration from `void` to `QUIC_STATUS`. **`datapath_raw.c` (`RawSocketDelete`):** Deletion is best-effort — logs a warning on failure rather than silently discarding the return value. **`datapath_raw_win.c`:** Creation path now checks the return value of `CxPlatDpRawPlumbRulesOnSocket` and calls `CxPlatRemoveSocket` to roll back on failure. Note: `CxPlatDpRawInterfaceRemoveRules` is intentionally left as `void` — rule removal failures are out of scope for this fix. ## Testing All 30 `DataPathTest` cases pass on Linux (`msquicplatformtest --gtest_filter="DataPathTest*"`). The XDP rule plumbing cleanup paths in `CxPlatDpRawPlumbRulesOnSocket` are Windows-only — `datapath_raw_xdp_win.c` is excluded from the Linux build entirely. The existing `QUIC_TEST_DATAPATH_HOOKS` infrastructure operates at the packet layer in `binding.c` and cannot reach `CxPlatDpRawInterfaceAddRules` in the XDP platform layer. Wiring failure injection there would require adding a new callback to the hook struct — happy to do this if the team considers it worth the effort. --------- Co-authored-by: Jack He <jackhe@microsoft.com>
#5747) (#5395) (#6050) ## Description Cherry picking: #5798 and #5747 ## Testing CI ## Documentation No --------- Co-authored-by: Guillaume Hetier <guhetier@microsoft.com> Co-authored-by: Michael Friesen <3517159+mtfriesen@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Fixes #5795
The XDP datapath can be configured to intercept packets based on QUIC Connection ID instead of local port.
This behavior existed in MsQuic but was not heavily exercised until recently.
One issue was that MsQuic always attempted to reserve UDP / TCP sockets for each application server process.
But for multiple server processes that may want to share a single port, we would run into port collision errors.
This PR adds support for CIBIR across multiple processes on the same port and document the behavior
Potential options to allow for multi-process port sharing:
Option chosen: 1
MsQuic's stance is that the application takes responsibility for book-keeping and protecting sharing shared local ports when using XDP + CIBIR.
What changed
Port protection options
Testing
A new DataPathTest was added.
Documentation
Settings.md