Skip to content

Commit db4bc55

Browse files
fix(xdp): propagate XDP rule plumbing failures to caller (#5948)
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>
1 parent 8b3c3f0 commit db4bc55

8 files changed

Lines changed: 212 additions & 20 deletions

File tree

src/generated/linux/datapath_raw.c.clog.h

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,26 @@ tracepoint(CLOG_DATAPATH_RAW_C, AllocFailure , arg2, arg3);\
4141

4242

4343

44+
/*----------------------------------------------------------
45+
// Decoder Ring for LibraryErrorStatus
46+
// [ lib] ERROR, %u, %s.
47+
// QuicTraceEvent(
48+
LibraryErrorStatus,
49+
"[ lib] ERROR, %u, %s.",
50+
PlumbStatus,
51+
"CxPlatDpRawPlumbRulesOnSocket (delete)");
52+
// arg2 = arg2 = PlumbStatus = arg2
53+
// arg3 = arg3 = "CxPlatDpRawPlumbRulesOnSocket (delete)" = arg3
54+
----------------------------------------------------------*/
55+
#ifndef _clog_4_ARGS_TRACE_LibraryErrorStatus
56+
#define _clog_4_ARGS_TRACE_LibraryErrorStatus(uniqueId, encoded_arg_string, arg2, arg3)\
57+
tracepoint(CLOG_DATAPATH_RAW_C, LibraryErrorStatus , arg2, arg3);\
58+
59+
#endif
60+
61+
62+
63+
4464
/*----------------------------------------------------------
4565
// Decoder Ring for DatapathRecv
4666
// [data][%p] Recv %u bytes (segment=%hu) Src=%!ADDR! Dst=%!ADDR!

src/generated/linux/datapath_raw.c.clog.h.lttng.h

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,29 @@ TRACEPOINT_EVENT(CLOG_DATAPATH_RAW_C, AllocFailure,
2424

2525

2626

27+
/*----------------------------------------------------------
28+
// Decoder Ring for LibraryErrorStatus
29+
// [ lib] ERROR, %u, %s.
30+
// QuicTraceEvent(
31+
LibraryErrorStatus,
32+
"[ lib] ERROR, %u, %s.",
33+
PlumbStatus,
34+
"CxPlatDpRawPlumbRulesOnSocket (delete)");
35+
// arg2 = arg2 = PlumbStatus = arg2
36+
// arg3 = arg3 = "CxPlatDpRawPlumbRulesOnSocket (delete)" = arg3
37+
----------------------------------------------------------*/
38+
TRACEPOINT_EVENT(CLOG_DATAPATH_RAW_C, LibraryErrorStatus,
39+
TP_ARGS(
40+
unsigned int, arg2,
41+
const char *, arg3),
42+
TP_FIELDS(
43+
ctf_integer(unsigned int, arg2, arg2)
44+
ctf_string(arg3, arg3)
45+
)
46+
)
47+
48+
49+
2750
/*----------------------------------------------------------
2851
// Decoder Ring for DatapathRecv
2952
// [data][%p] Recv %u bytes (segment=%hu) Src=%!ADDR! Dst=%!ADDR!

src/platform/datapath_raw.c

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,19 @@ RawSocketDelete(
174174
_In_ CXPLAT_SOCKET_RAW* Socket
175175
)
176176
{
177-
CxPlatDpRawPlumbRulesOnSocket(Socket, FALSE);
177+
//
178+
// Rule removal on socket deletion is best-effort. A failure here means
179+
// some XDP rules may linger until the interface is reinitialized, but
180+
// there is nothing useful we can do at this point.
181+
//
182+
QUIC_STATUS PlumbStatus = CxPlatDpRawPlumbRulesOnSocket(Socket, FALSE);
183+
if (QUIC_FAILED(PlumbStatus)) {
184+
QuicTraceEvent(
185+
LibraryErrorStatus,
186+
"[ lib] ERROR, %u, %s.",
187+
PlumbStatus,
188+
"CxPlatDpRawPlumbRulesOnSocket (delete)");
189+
}
178190
CxPlatRemoveSocket(&Socket->RawDatapath->SocketPool, Socket);
179191
CxPlatRundownReleaseAndWait(&Socket->RawRundown);
180192
if (Socket->PausedTcpSend) {

src/platform/datapath_raw.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ CxPlatDpRawUpdatePollingIdleTimeout(
148148
// that it should update any filtering rules as necessary.
149149
//
150150
_IRQL_requires_max_(PASSIVE_LEVEL)
151-
void
151+
QUIC_STATUS
152152
CxPlatDpRawPlumbRulesOnSocket(
153153
_In_ CXPLAT_SOCKET_RAW* Socket,
154154
_In_ BOOLEAN IsCreated

src/platform/datapath_raw_win.c

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,28 @@ RawSocketCreateUdp(
170170
goto Error;
171171
}
172172

173-
CxPlatDpRawPlumbRulesOnSocket(Socket, TRUE);
173+
Status = CxPlatDpRawPlumbRulesOnSocket(Socket, TRUE);
174+
if (QUIC_FAILED(Status)) {
175+
//
176+
// CxPlatDpRawPlumbRulesOnSocket(TRUE) stops at the first interface
177+
// where rule installation fails, so some interfaces may already have
178+
// partial state (rules installed or port bits set) that must be rolled
179+
// back. Reuse the IsCreated=FALSE path for best-effort cleanup; it is
180+
// safe to call against interfaces where nothing was installed (no-op
181+
// for those). Cleanup failures are logged but do not change the
182+
// returned error.
183+
//
184+
QUIC_STATUS CleanupStatus = CxPlatDpRawPlumbRulesOnSocket(Socket, FALSE);
185+
if (QUIC_FAILED(CleanupStatus)) {
186+
QuicTraceEvent(
187+
LibraryErrorStatus,
188+
"[ lib] ERROR, %u, %s.",
189+
CleanupStatus,
190+
"CxPlatDpRawPlumbRulesOnSocket cleanup");
191+
}
192+
CxPlatRemoveSocket(&Raw->SocketPool, Socket);
193+
goto Error;
194+
}
174195

175196
Error:
176197

src/platform/datapath_raw_xdp_win.c

Lines changed: 76 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,7 @@ CxPlatDpRawInterfaceInitialize(
853853

854854
_IRQL_requires_max_(PASSIVE_LEVEL)
855855
_Requires_lock_held_(Interface->RuleLock)
856-
void
856+
QUIC_STATUS
857857
CxPlatDpRawInterfaceUpdateRules(
858858
_In_ XDP_INTERFACE* Interface
859859
)
@@ -864,6 +864,8 @@ CxPlatDpRawInterfaceUpdateRules(
864864
.SubLayer = XDP_HOOK_INSPECT,
865865
};
866866

867+
QUIC_STATUS ReturnStatus = QUIC_STATUS_SUCCESS;
868+
867869
for (uint32_t i = 0; i < Interface->QueueCount; i++) {
868870

869871
CXPLAT_QUEUE* Queue = &Interface->Queues[i];
@@ -882,16 +884,14 @@ CxPlatDpRawInterfaceUpdateRules(
882884
Interface->RuleCount,
883885
&NewRxProgram);
884886
if (QUIC_FAILED(Status)) {
885-
//
886-
// TODO - Figure out how to better handle failure and revert changes.
887-
// This will likely require working with XDP to get an improved API;
888-
// possibly to update all queues at once.
889-
//
890887
QuicTraceEvent(
891888
LibraryErrorStatus,
892889
"[ lib] ERROR, %u, %s.",
893890
Status,
894891
"XdpCreateProgram");
892+
if (QUIC_SUCCEEDED(ReturnStatus)) {
893+
ReturnStatus = Status;
894+
}
895895
continue;
896896
}
897897

@@ -901,10 +901,18 @@ CxPlatDpRawInterfaceUpdateRules(
901901

902902
Queue->RxProgram = NewRxProgram;
903903
}
904+
905+
return ReturnStatus;
904906
}
905907

908+
//
909+
// Takes ownership of any PortSet buffers in the input rules. On any failure
910+
// path (pre-copy or post-copy via CxPlatDpRawInterfaceUpdateRules), the
911+
// PortSet buffer is freed by either this function or the interface — the
912+
// caller must not free it.
913+
//
906914
_IRQL_requires_max_(PASSIVE_LEVEL)
907-
void
915+
QUIC_STATUS
908916
CxPlatDpRawInterfaceAddRules(
909917
_In_ XDP_INTERFACE* Interface,
910918
_In_reads_(Count) const XDP_RULE* Rules,
@@ -914,6 +922,8 @@ CxPlatDpRawInterfaceAddRules(
914922
#pragma warning(push)
915923
#pragma warning(disable:6386) // Buffer overrun while writing to 'NewRules' - FALSE POSITIVE
916924

925+
QUIC_STATUS Status;
926+
917927
CxPlatLockAcquire(&Interface->RuleLock);
918928
// TODO - Don't always allocate a new array?
919929

@@ -923,7 +933,18 @@ CxPlatDpRawInterfaceAddRules(
923933
"[ lib] ERROR, %s.",
924934
"No more room for rules");
925935
CxPlatLockRelease(&Interface->RuleLock);
926-
return;
936+
//
937+
// We own any PortSet buffers in the input rules; free them since
938+
// they were not transferred to Interface->Rules.
939+
//
940+
for (uint8_t i = 0; i < Count; i++) {
941+
if (Rules[i].Pattern.IpPortSet.PortSet.PortSet) {
942+
CxPlatFree(
943+
(uint8_t*)Rules[i].Pattern.IpPortSet.PortSet.PortSet,
944+
PORT_SET_TAG);
945+
}
946+
}
947+
return QUIC_STATUS_BUFFER_TOO_SMALL;
927948
}
928949

929950
const size_t OldSize = sizeof(XDP_RULE) * (size_t)Interface->RuleCount;
@@ -937,7 +958,18 @@ CxPlatDpRawInterfaceAddRules(
937958
"XDP_RULE",
938959
NewSize);
939960
CxPlatLockRelease(&Interface->RuleLock);
940-
return;
961+
//
962+
// We own any PortSet buffers in the input rules; free them since
963+
// they were not transferred to Interface->Rules.
964+
//
965+
for (uint8_t i = 0; i < Count; i++) {
966+
if (Rules[i].Pattern.IpPortSet.PortSet.PortSet) {
967+
CxPlatFree(
968+
(uint8_t*)Rules[i].Pattern.IpPortSet.PortSet.PortSet,
969+
PORT_SET_TAG);
970+
}
971+
}
972+
return QUIC_STATUS_OUT_OF_MEMORY;
941973
}
942974

943975
if (Interface->RuleCount > 0) {
@@ -952,11 +984,13 @@ CxPlatDpRawInterfaceAddRules(
952984
}
953985
Interface->Rules = NewRules;
954986

955-
CxPlatDpRawInterfaceUpdateRules(Interface);
987+
Status = CxPlatDpRawInterfaceUpdateRules(Interface);
956988

957989
CxPlatLockRelease(&Interface->RuleLock);
958990

959991
#pragma warning(pop)
992+
993+
return Status;
960994
}
961995

962996
_IRQL_requires_max_(PASSIVE_LEVEL)
@@ -1423,12 +1457,13 @@ CxPlatDpRawClearPortBit(
14231457
}
14241458

14251459
_IRQL_requires_max_(PASSIVE_LEVEL)
1426-
void
1460+
QUIC_STATUS
14271461
CxPlatDpRawPlumbRulesOnSocket(
14281462
_In_ CXPLAT_SOCKET_RAW* Socket,
14291463
_In_ BOOLEAN IsCreated
14301464
)
14311465
{
1466+
QUIC_STATUS Status = QUIC_STATUS_SUCCESS;
14321467
XDP_DATAPATH* Xdp = (XDP_DATAPATH*)Socket->RawDatapath;
14331468
if (Socket->Wildcard) {
14341469
XDP_RULE Rules[5] = {0};
@@ -1504,7 +1539,17 @@ CxPlatDpRawPlumbRulesOnSocket(
15041539
for (Entry = Xdp->Interfaces.Flink; Entry != &Xdp->Interfaces; Entry = Entry->Flink) {
15051540
XDP_INTERFACE* Interface = CONTAINING_RECORD(Entry, XDP_INTERFACE, Link);
15061541
if (IsCreated) {
1507-
CxPlatDpRawInterfaceAddRules(Interface, Rules, RulesSize);
1542+
QUIC_STATUS AddStatus = CxPlatDpRawInterfaceAddRules(Interface, Rules, RulesSize);
1543+
if (QUIC_FAILED(AddStatus)) {
1544+
//
1545+
// Stop on first failure and propagate. The caller is
1546+
// responsible for invoking this function again with
1547+
// IsCreated=FALSE to roll back any rules already
1548+
// installed on previous interfaces.
1549+
//
1550+
Status = AddStatus;
1551+
break;
1552+
}
15081553
} else {
15091554
CxPlatDpRawInterfaceRemoveRules(Interface, Rules, RulesSize);
15101555
}
@@ -1566,14 +1611,29 @@ CxPlatDpRawPlumbRulesOnSocket(
15661611
"Allocation of '%s' failed. (%llu bytes)",
15671612
"PortSet",
15681613
XDP_PORT_SET_BUFFER_SIZE);
1569-
return;
1614+
Status = QUIC_STATUS_OUT_OF_MEMORY;
1615+
break;
15701616
}
15711617
CxPlatDpRawSetPortBit(
15721618
(uint8_t*)NewRule.Pattern.IpPortSet.PortSet.PortSet,
15731619
Socket->LocalAddress.Ipv4.sin_port);
15741620
memcpy(
15751621
&NewRule.Pattern.IpPortSet.Address, IpAddress, IpAddressSize);
1576-
CxPlatDpRawInterfaceAddRules(Interface, &NewRule, 1);
1622+
QUIC_STATUS AddStatus = CxPlatDpRawInterfaceAddRules(Interface, &NewRule, 1);
1623+
if (QUIC_FAILED(AddStatus)) {
1624+
Status = AddStatus;
1625+
//
1626+
// CxPlatDpRawInterfaceAddRules takes ownership of the
1627+
// PortSet buffer on every path — including failure —
1628+
// so we don't free it here.
1629+
//
1630+
// Stop on first failure and propagate. The caller is
1631+
// responsible for invoking this function again with
1632+
// IsCreated=FALSE to roll back any port bits already
1633+
// set on previous interfaces.
1634+
//
1635+
break;
1636+
}
15771637
}
15781638
} else {
15791639
//
@@ -1588,6 +1648,8 @@ CxPlatDpRawPlumbRulesOnSocket(
15881648
}
15891649
}
15901650
}
1651+
1652+
return Status;
15911653
}
15921654

15931655
_IRQL_requires_max_(DISPATCH_LEVEL)

src/platform/datapath_xplat.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,17 +162,18 @@ CxPlatSocketCreateUdp(
162162
QuicTraceLogVerbose(
163163
RawSockCreateFail,
164164
"[sock] Failed to create raw socket, status:%d", Status);
165-
BOOLEAN IsWildcardAddr = Config->LocalAddress == NULL || QuicAddrIsWildCard(Config->LocalAddress);
166-
if (IsWildcardAddr && RequiresQtip) {
165+
BOOLEAN IsServerSocket = !(*NewSocket)->HasFixedRemoteAddress;
166+
if (IsServerSocket && RequiresQtip) {
167167
//
168168
// This retry loop is purely for QTIP listener sockets that try to reserve both a UDP/TCP port,
169169
// which may run into a port collision for TCP if the UDP ephemeral port collides with something
170170
// in the TCP pool. So just try it again.
171171
//
172172
CxPlatSocketDelete(*NewSocket);
173+
*NewSocket = NULL;
173174
continue;
174175
}
175-
if ((!RequiresQtip && !CibirRequested) || ((*NewSocket)->HasFixedRemoteAddress && CibirRequested && !RequiresQtip)) {
176+
if ((!RequiresQtip && !CibirRequested) || (!IsServerSocket && CibirRequested && !RequiresQtip)) {
176177
//
177178
// Allow fallback to OS UDP sockets in these 2 cases only:
178179
// - XDP with no QTIP and no CIBIR.
@@ -187,6 +188,7 @@ CxPlatSocketCreateUdp(
187188
Status = QUIC_STATUS_SUCCESS;
188189
} else {
189190
CxPlatSocketDelete(*NewSocket);
191+
*NewSocket = NULL;
190192
}
191193
goto Error;
192194
}
@@ -195,6 +197,7 @@ CxPlatSocketCreateUdp(
195197
ErrNoXdpForQtip,
196198
"[sock] Error: app requested QTIP but XDP not enabled/available/initialized.");
197199
CxPlatSocketDelete(*NewSocket);
200+
*NewSocket = NULL;
198201
Status = QUIC_STATUS_INVALID_STATE;
199202
goto Error;
200203
} else if (CibirRequested) {

0 commit comments

Comments
 (0)