fix(xdp): propagate XDP rule plumbing failures to caller

[MarkedMuichiro]

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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.21%. Comparing base (d505e2a) to head (9f9dce0).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5948      +/-   ##
==========================================
- Coverage   85.93%   84.21%   -1.73%     
==========================================
  Files          60       60              
  Lines       18726    18730       +4     
==========================================
- Hits        16093    15774     -319     
- Misses       2633     2956     +323     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MarkedMuichiro]

Simplified the conditional — a success on a later interface no longer overwrites an earlier failure. Also fixed a potential use-after-free in the non-Wildcard branch: the PortSet free is now guarded by QUIC_STATUS_OUT_OF_MEMORY and QUIC_STATUS_BUFFER_TOO_SMALL only, since those are the two pre-copy failure paths where the caller still owns the buffer. On XdpCreateProgram failure the rule is already in Interface->Rules and the interface owns the buffer.

[ProjectsByJackHe]

My biggest concern (when I created this issue) is partial instantiation...... and the non-determinism of it all. This isn't an issue with your PR, I am just braindumping the current problem we have and hopefully go for a more complete + deterministic solution.

Today, MsQuic creates 1 program per interface per queue, and each program has its own rules engine. So, if a system has N interfaces, and each interface has K queues, MsQuic will create N * K XDP programs, and tries to replicate the same ruleset to those N * K programs.

Now if we fail to plumb rules on some programs, the flawed logic is we continue on failure and ignore it. It's good that your PR at least adds an error status, but that doesn't solve the partial instantiation, and the necessary cleanup. So, continuing on failure ideally should be avoided. We have to have a coherent cleanup strategy on failure.

Ideally, we have something atomic. Either all programs get their rules plumbed, or none do and the application can try it again later.

[MarkedMuichiro]

I can take care of the two concrete items — adding the best-effort comment and warning log in RawSocketDelete, and cleaning up already-installed rules on failure in the Wildcard branch before returning. For the atomic all-or-nothing guarantee across N×K programs, that sounds like it goes beyond what this PR can reasonably solve and might need a separate issue and possibly XDP API changes. Should I scope this PR to those cleanup improvements and track the larger problem separately, or would you all prefer a different approach?

[ProjectsByJackHe]

I can take care of the two concrete items — adding the best-effort comment and warning log in RawSocketDelete, and cleaning up already-installed rules on failure in the Wildcard branch before returning. For the atomic all-or-nothing guarantee across N×K programs, that sounds like it goes beyond what this PR can reasonably solve and might need a separate issue and possibly XDP API changes. Should I scope this PR to those cleanup improvements and track the larger problem separately, or would you all prefer a different approach?

That's fair, sorry for the scope creep. But the gist of it is, we should try to clean up at least on a best effort basis partially plumbed interfaces/queues. Maybe "guarantee" is a strong word :)

[MarkedMuichiro]

I can take care of the two concrete items — adding the best-effort comment and warning log in RawSocketDelete, and cleaning up already-installed rules on failure in the Wildcard branch before returning. For the atomic all-or-nothing guarantee across N×K programs, that sounds like it goes beyond what this PR can reasonably solve and might need a separate issue and possibly XDP API changes. Should I scope this PR to those cleanup improvements and track the larger problem separately, or would you all prefer a different approach?

That's fair, sorry for the scope creep. But the gist of it is, we should try to clean up at least on a best effort basis partially plumbed interfaces/queues. Maybe "guarantee" is a strong word :)

Done — pushed the best-effort cleanup. On failure in the Wildcard branch the loop now breaks at the first failure and rolls back already-configured interfaces. Also added the warning log in RawSocketDelete. Best-effort is plenty :)

[ProjectsByJackHe]

There are still a couple of CLOG failures - you might need to clear auto-generated sidecar files then run update-sidecar.ps1 again.

[ProjectsByJackHe]

There is currently no coverage for the cleanup routines and your error handling code.

If even as a sanity check, can you verify that there is no crash or undefined behavior when your failure path runs?

You can hard-code a conditional to go into that path through the usage of MsQuic Test Hooks. It would be ideal if this can be added to our automated CI.

But at a bare minimum, can you verify manually the cleanup code works as expected + update PR description with how you tested this?

[MarkedMuichiro]

There is currently no coverage for the cleanup routines and your error handling code.

If even as a sanity check, can you verify that there is no crash or undefined behavior when your failure path runs?

You can hard-code a conditional to go into that path through the usage of MsQuic Test Hooks. It would be ideal if this can be added to our automated CI.

But at a bare minimum, can you verify manually the cleanup code works as expected + update PR description with how you tested this?

I looked into using QUIC_TEST_DATAPATH_HOOKS but the existing callbacks (Create, Send, Receive, GetLocalAddress, GetRemoteAddress) operate at the packet layer in binding.c and can't reach CxPlatDpRawInterfaceAddRules in the XDP platform layer — so there's no existing hook point to inject a failure there. Wiring one in would require adding a new callback to the hook struct. Happy to do that if the team considers it worth it, just want guidance on the right approach before touching the hook infrastructure. In the meantime I ran all 30 DataPath tests on Linux and they pass cleanly, and updated the PR description with testing notes.

[ProjectsByJackHe]

There is currently no coverage for the cleanup routines and your error handling code.
If even as a sanity check, can you verify that there is no crash or undefined behavior when your failure path runs?
You can hard-code a conditional to go into that path through the usage of MsQuic Test Hooks. It would be ideal if this can be added to our automated CI.
But at a bare minimum, can you verify manually the cleanup code works as expected + update PR description with how you tested this?

I looked into using QUIC_TEST_DATAPATH_HOOKS but the existing callbacks (Create, Send, Receive, GetLocalAddress, GetRemoteAddress) operate at the packet layer in binding.c and can't reach CxPlatDpRawInterfaceAddRules in the XDP platform layer — so there's no existing hook point to inject a failure there. Wiring one in would require adding a new callback to the hook struct. Happy to do that if the team considers it worth it, just want guidance on the right approach before touching the hook infrastructure. In the meantime I ran all 30 DataPath tests on Linux and they pass cleanly, and updated the PR description with testing notes.

I see. If it's do-able for you to add those hook callbacks to inject those failures in the Datapath, that would be great. If it's too much, then we can save it for another PR. But at least having a manual pass through the codepath to add some coverage is needed. BTW, you said "ran 30 DataPath tests on Linux" --- I am not sure how this relates to the current changes that are only in the WinUser xdp path. Did you mean windows?

[MarkedMuichiro]

There is currently no coverage for the cleanup routines and your error handling code.
If even as a sanity check, can you verify that there is no crash or undefined behavior when your failure path runs?
You can hard-code a conditional to go into that path through the usage of MsQuic Test Hooks. It would be ideal if this can be added to our automated CI.
But at a bare minimum, can you verify manually the cleanup code works as expected + update PR description with how you tested this?

I looked into using QUIC_TEST_DATAPATH_HOOKS but the existing callbacks (Create, Send, Receive, GetLocalAddress, GetRemoteAddress) operate at the packet layer in binding.c and can't reach CxPlatDpRawInterfaceAddRules in the XDP platform layer — so there's no existing hook point to inject a failure there. Wiring one in would require adding a new callback to the hook struct. Happy to do that if the team considers it worth it, just want guidance on the right approach before touching the hook infrastructure. In the meantime I ran all 30 DataPath tests on Linux and they pass cleanly, and updated the PR description with testing notes.

I see. If it's do-able for you to add those hook callbacks to inject those failures in the Datapath, that would be great. If it's too much, then we can save it for another PR. But at least having a manual pass through the codepath to add some coverage is needed. BTW, you said "ran 30 DataPath tests on Linux" --- I am not sure how this relates to the current changes that are only in the WinUser xdp path. Did you mean windows?

Yes, I did mean Windows, sorry about that. I dug a bit deeper and found that the layering, datapath_raw_xdp_win.c has no include path to core/library.h so accessing MsQuicLib.TestDatapathHooks from the platform layer would invert the dependency graph — that's a real architectural violation. Here are two viable options:

Option A: Add a static function pointer QUIC_STATUS (*CxPlatDpRawInterfaceAddRulesHook)(void) = NULL in datapath_raw_xdp_win.c itself, consulted at the top of CxPlatDpRawInterfaceAddRules under #if QUIC_TEST_DATAPATH_HOOKS_ENABLED. Stays entirely within the platform layer, zero breakage elsewhere, covers all failure cases.

Option B: Use the existing CxPlatSetAllocFailDenominator mechanism to trigger the QUIC_STATUS_OUT_OF_MEMORY branch inside CxPlatDpRawInterfaceAddRules with zero new code — only covers the OOM path but may be sufficient.

Which approach would you prefer? I'm happy to consider other approaches as well.

[ProjectsByJackHe]

There is currently no coverage for the cleanup routines and your error handling code.
If even as a sanity check, can you verify that there is no crash or undefined behavior when your failure path runs?
You can hard-code a conditional to go into that path through the usage of MsQuic Test Hooks. It would be ideal if this can be added to our automated CI.
But at a bare minimum, can you verify manually the cleanup code works as expected + update PR description with how you tested this?

I looked into using QUIC_TEST_DATAPATH_HOOKS but the existing callbacks (Create, Send, Receive, GetLocalAddress, GetRemoteAddress) operate at the packet layer in binding.c and can't reach CxPlatDpRawInterfaceAddRules in the XDP platform layer — so there's no existing hook point to inject a failure there. Wiring one in would require adding a new callback to the hook struct. Happy to do that if the team considers it worth it, just want guidance on the right approach before touching the hook infrastructure. In the meantime I ran all 30 DataPath tests on Linux and they pass cleanly, and updated the PR description with testing notes.

I see. If it's do-able for you to add those hook callbacks to inject those failures in the Datapath, that would be great. If it's too much, then we can save it for another PR. But at least having a manual pass through the codepath to add some coverage is needed. BTW, you said "ran 30 DataPath tests on Linux" --- I am not sure how this relates to the current changes that are only in the WinUser xdp path. Did you mean windows?

Yes, I did mean Windows, sorry about that. I dug a bit deeper and found that the layering, datapath_raw_xdp_win.c has no include path to core/library.h so accessing MsQuicLib.TestDatapathHooks from the platform layer would invert the dependency graph — that's a real architectural violation. Here are two viable options:

Option A: Add a static function pointer QUIC_STATUS (*CxPlatDpRawInterfaceAddRulesHook)(void) = NULL in datapath_raw_xdp_win.c itself, consulted at the top of CxPlatDpRawInterfaceAddRules under #if QUIC_TEST_DATAPATH_HOOKS_ENABLED. Stays entirely within the platform layer, zero breakage elsewhere, covers all failure cases.

Option B: Use the existing CxPlatSetAllocFailDenominator mechanism to trigger the QUIC_STATUS_OUT_OF_MEMORY branch inside CxPlatDpRawInterfaceAddRules with zero new code — only covers the OOM path but may be sufficient.

Which approach would you prefer? I'm happy to consider other approaches as well.

Yeah you can just trigger the OOM branch with CxPlatSetAllocFailDenominator. The specific failure doesn't matter, I just want to make sure that the cleanup code runs and no egregious errors.

[ProjectsByJackHe]

A bunch of BVT CI failures @MarkedMuichiro

For the CLOG errors, have you tried just cloning the repo to your local environment instead of using Github codespaces?
Our scripts generally are the most stable in a windows environment.

[guhetier]

For the CLOG failures, if running the script causes issues, the CI failure should report the diff and you can fix it by hand.
It might be some whitespace / return character problem.
https://github.com/microsoft/msquic/actions/runs/24915489496/job/73243800270?pr=5948

[MarkedMuichiro]

Pushed the trailing-space fix for the CLOG diff. Workflows are waiting on maintainer approval to run — could either of you approve when you get a chance @guhetier @ProjectsByJackHe ?

[MarkedMuichiro]

XdpRuleAddOomCleanup is failing on the Windows BVTs because Socket.GetInitStatus() returned success — the CxPlatSetAllocFailDenominator(-2) value doesn't reliably fail the rule array allocation in this CI environment. I think -2 was based on assumed allocation ordering that may differ on Windows XDP. I have two options: (1) reduce the test to just verify no crash regardless of init status, or (2) try a different denominator value. Since the goal of the test was just to ensure the cleanup path doesn't crash, would option 1 be acceptable? It would change ASSERT_TRUE(QUIC_FAILED(...)) to just running the code and asserting no crash.

[guhetier]

XdpRuleAddOomCleanup is failing on the Windows BVTs because Socket.GetInitStatus() returned success — the CxPlatSetAllocFailDenominator(-2) value doesn't reliably fail the rule array allocation in this CI environment. I think -2 was based on assumed allocation ordering that may differ on Windows XDP. I have two options: (1) reduce the test to just verify no crash regardless of init status, or (2) try a different denominator value. Since the goal of the test was just to ensure the cleanup path doesn't crash, would option 1 be acceptable? It would change ASSERT_TRUE(QUIC_FAILED(...)) to just running the code and asserting no crash.

A test that depends on the order / number of memory allocations will be brittle, I don't think we should merge it.
Adding a unit test validating the XDP path can be initialized successfully is probably useful if we don't already have one, but does little to help validating this specific change.

A manual validation, modifying the datapath code to pretend an allocation fails, and checking the error path execute as expected would be welcome.

@ProjectsByJackHe since it can be difficult to setup XDP, maybe you can assist with running this validation?

[ProjectsByJackHe]

Hey @MarkedMuichiro, I created MarkedMuichiro#1 to fix up the BVT errors and confirmed from a local debugger that the added cleanup path has code coverage from my fixed up DataPathTest. Once we get that merged, and clean up some nits from @guhetier, this PR should be good to go!

[MarkedMuichiro]

Pushed. Three changes:

• Reverted CxPlatDpRawInterfaceUpdateRules back to void — per-queue XdpCreateProgram failures are now best-effort as before.
• Removed the obsolete TODO comment in the XdpCreateProgram failure branch.
• Added a comment above #ifdef DEBUG in DataPathTest.cpp explaining why the test must be DEBUG-only (CxPlatSetAllocFailDenominator is debug-only).

Also includes Jack's PR #1 changes for the simplified cleanup approach — RawSocketCreateUdp now invokes PlumbRulesOnSocket(socket, FALSE) for best-effort cleanup on install failure, and the test was updated to use a connected QTIP socket so the failure propagates deterministically.

[ProjectsByJackHe]

It seems a bit too much got reverted after my helper PR got merged into this topic branch.

[guhetier]

Reverted CxPlatDpRawInterfaceUpdateRules back to void — per-queue XdpCreateProgram failures are now best-effort as before.

I'm confused by that part, why? Am I missing a problem with still returning an error from CxPlatDpRawInterfaceUpdateRules and keeping the rollback as it is done now?

Resolve all comments first.

[ProjectsByJackHe]

• Reverted CxPlatDpRawInterfaceUpdateRules back to void — per-queue XdpCreateProgram failures are now best-effort as before.

@MarkedMuichiro we still need to propagate this up. You can just set the HEAD of this PR to equal the HEAD of #5968.

To clear up the objective, our goal is to notify the app whenever any problems occur when setting up XDP for data paths that requires it (i.e. does not have a fallback option (QTIP, CIBIR))

We can have the cleanup be best-effort and non-deterministic. Surprisingly, PlumbRules(FALSE) is a reasonably robust cleanup routine, even for partially initialized programs (if XdpCreateProgram fails).

[MarkedMuichiro]

Updated branch to align with #5968 — CxPlatDpRawInterfaceUpdateRules and CxPlatDpRawInterfaceAddRules propagate the error code, and the simplified cleanup pattern remains: on install failure, RawSocketCreateUdp invokes PlumbRulesOnSocket(socket, FALSE) for best-effort rollback before returning the error to the caller.

Also addressed the two remaining nits:

• Removed the obsolete TODO comment in the XdpCreateProgram failure branch
• Added a comment above #ifdef DEBUG in DataPathTest.cpp explaining the test is DEBUG-only because of CxPlatSetAllocFailDenominator

Sorry for the earlier confusion @guhetier — I misread your previous comment about UpdateRules returning void. Thanks for the helper PR @ProjectsByJackHe.